Skip to content

[Repo Assist] perf(logger): sanitize RPC payload once for text+markdown previews#8722

Merged
lpcox merged 2 commits into
mainfrom
repo-assist/perf-rpc-single-sanitize-20260705-6c5a6db8fa2d730a
Jul 5, 2026
Merged

[Repo Assist] perf(logger): sanitize RPC payload once for text+markdown previews#8722
lpcox merged 2 commits into
mainfrom
repo-assist/perf-rpc-single-sanitize-20260705-6c5a6db8fa2d730a

Conversation

@github-actions

@github-actions github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

🤖 This is an automated pull request from Repo Assist, an AI assistant.

Summary

Each RPC message is logged in two preview formats — a text log (10 KB preview) and a markdown log (512-byte preview). Previously logRPCMessageToAll called newRPCMessageInfo twice, each of which called truncateAndSanitizesanitize.SanitizeString. SanitizeString runs 10 compiled regexp.Regexp.ReplaceAllStringFunc calls over the full payload, so every RPC hop paid that cost twice.

Root Cause

Before: two full sanitization passes per RPC hop:

infoText     := newRPCMessageInfo(..., MaxPayloadPreviewLengthText)     // calls SanitizeString
infoMarkdown := newRPCMessageInfo(..., MaxPayloadPreviewLengthMarkdown) // calls SanitizeString again

Fix

  • Added newRPCMessageInfoFromSanitized: builds RPCMessageInfo from an already-sanitized string, only truncating.
  • Added truncateSanitized: thin wrapper over util.Truncate to signal "already sanitized, just truncate".
  • Updated logRPCMessageToAll to call sanitize.SanitizeString once and share the result for both previews.
  • Rewrote formatRPCMessage to use strings.Builder + strconv.Itoa instead of []string + append + strings.Join + fmt.Sprintf, eliminating the intermediate slice allocation.

After: one sanitization pass shared between both preview lengths:

sanitized    := sanitize.SanitizeString(string(payload))
infoText     := newRPCMessageInfoFromSanitized(..., sanitized, MaxPayloadPreviewLengthText)
infoMarkdown := newRPCMessageInfoFromSanitized(..., sanitized, MaxPayloadPreviewLengthMarkdown)

Trade-offs

  • No behaviour change: output is identical.
  • The sanitization contract is preserved — newRPCMessageInfoFromSanitized is only called from logRPCMessageToAll after an explicit sanitize.SanitizeString call.
  • newRPCMessageInfo is retained for future callers that provide a raw payload.

Test Status

⚠️ Outbound network access to proxy.golang.org is blocked in this sandbox, preventing go test / make agent-finished from downloading module dependencies. The changes were reviewed for correctness against existing tests:

  • TestFormatRPCMessage — exercises formatRPCMessage via RPCMessageInfo; builder-based rewrite produces identical output.
  • TestLogRPCRequest, TestLogRPCResponse, TestLogRPCRequestWithSecrets, TestLogRPCRequestPayloadTruncation — exercise logRPCMessageToAll through the public API; behaviour is unchanged.
  • TestTruncateAndSanitize — tests truncateAndSanitize which is unchanged.

Warning

Firewall blocked 2 domains

The following domains were blocked by the firewall during workflow execution:

  • awmgmcpg
  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by Repo Assist · 464.4 AIC · ⊞ 10.8K ·
Comment /repo-assist to run again

Add this agentic workflow to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@851905c06e905bf362a9f6cc54f912e3df747d55

RPC messages are logged in two formats (text with a 10KB preview and
markdown with a 512-byte preview). Previously, logRPCMessageToAll called
newRPCMessageInfo twice, which called truncateAndSanitize → SanitizeString
twice on the same payload. SanitizeString runs 10 compiled regex patterns
over the full payload, so every RPC hop paid that cost twice.

This change:
- Adds newRPCMessageInfoFromSanitized: builds RPCMessageInfo from an
  already-sanitized string, only truncating to the desired length.
- Adds truncateSanitized: thin wrapper over util.Truncate that signals
  "this is already sanitized; just truncate".
- Updates logRPCMessageToAll to call sanitize.SanitizeString once, then
  share the sanitized string for both the text and markdown info structs.
- Rewrites formatRPCMessage to use strings.Builder + strconv.Itoa instead
  of []string + append + strings.Join + fmt.Sprintf, eliminating the
  intermediate slice allocation.

No behaviour change: output is identical. The sanitization contract is
preserved because newRPCMessageInfoFromSanitized is only called from
logRPCMessageToAll after an explicit sanitize.SanitizeString call.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox lpcox marked this pull request as ready for review July 5, 2026 14:36
@lpcox lpcox requested review from Copilot July 5, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces per-RPC logging overhead in the internal/logger package by ensuring payload sanitization is performed once per message and reused across multiple preview formats, and by reducing allocations in the text RPC formatter.

Changes:

  • Added a sanitized-payload path (newRPCMessageInfoFromSanitized + truncateSanitized) so text + markdown previews share the same sanitization work.
  • Updated logRPCMessageToAll to sanitize once and then truncate to the two configured preview lengths.
  • Rewrote formatRPCMessage to use strings.Builder + strconv.Itoa to avoid intermediate slice allocations.
Show a summary per file
File Description
internal/logger/rpc_logger.go Sanitizes RPC payload once and reuses it for text/markdown previews via a new helper constructor.
internal/logger/rpc_format.go Adds truncateSanitized and optimizes formatRPCMessage to reduce allocations.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Low

@lpcox

lpcox commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

@copilot fix failing ci

Copilot AI commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

@copilot fix failing ci

Fixed in 0ee12ef. The CI failure was the unused linter flagging newRPCMessageInfo — the function was retained after the refactor but never called. Removed it.

Copilot finished work on behalf of lpcox July 5, 2026 15:08
Copilot AI requested a review from lpcox July 5, 2026 15:08
@lpcox lpcox merged commit b2be31c into main Jul 5, 2026
23 checks passed
@lpcox lpcox deleted the repo-assist/perf-rpc-single-sanitize-20260705-6c5a6db8fa2d730a branch July 5, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants