Remove use of html/template for DebugReports#118
Conversation
Leveraging templates prevents dead code elimination for consumers of the trace library. The old method for generating the debug reports has been moved into trace_test.go and exposed via OldDebugReport functions so that the new handrolled reports can be validated against the templated version.
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| require.Equal(t, OldProxyErrorDebugReport(test.err), test.err.DebugReport()) |
There was a problem hiding this comment.
Capture the output of the old functions and ditch them? I don't see much reason to keep all of that around, after this change lands this is it anyway.
There was a problem hiding this comment.
I considered this but thought it would be better to leave the old behavior around to validate any future changes or corner cases.
There was a problem hiding this comment.
Maybe move the old funcs to their own file, then? trace_olddebug_test.go?
rosstimothy
requested review from
Copilot,
espadolini and
zmb3
and removed request for
zmb3
| sb.WriteString("User Message: ") | ||
| sb.WriteString(html.EscapeString(wrappedErr.UserMessage())) | ||
| sb.WriteRune('\n') | ||
| } else { | ||
| sb.WriteString("User Message: ") | ||
| sb.WriteString(html.EscapeString(wrappedErr.UserMessage())) | ||
| } |
There was a problem hiding this comment.
There's no way the (lack of) trailing newline depending on Caught is relevant, right? Surely that's just an artifact of how we wrote reportTemplateText, and we can do the same thing in both cases here instead.
There was a problem hiding this comment.
🤷 I was trying to limit the behavioral changes to a minimum here.
There was a problem hiding this comment.
Pull request overview
Removes the dependency on html/template from DebugReport generation to allow dead code elimination for downstream consumers, while keeping output parity validated via tests.
Changes:
- Replaced templated DebugReport rendering with hand-rolled string building (with HTML escaping) for
TraceErrandproxyError. - Moved the legacy template-based DebugReport generation into tests and added parity tests comparing old vs new outputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| trace.go | Swaps template-based debug report formatting for manual formatting + escaping and removes html/template dependency. |
| trace_test.go | Reintroduces the old template logic in tests only and adds golden/parity tests to ensure identical output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Inline the html strings.Replacer for more efficient debug reports.
This was validated by adding BenchmarkDebugReport. There is still
more room for improvement in the DebugReport implementations but
any further improvements are for a follow up change.
```
benchstat old.txt new.txt
goos: darwin
goarch: arm64
pkg: github.com/gravitational/trace
cpu: Apple M2 Pro
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
DebugReport-12 22.53µ ± 3% 21.49µ ± 3% -4.61% (p=0.001 n=10)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
DebugReport-12 2.312Ki ± 0% 1.888Ki ± 0% -18.37% (p=0.000 n=10)
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
DebugReport-12 38.00 ± 0% 29.00 ± 0% -23.68% (p=0.000 n=10)
```
|
@espadolini @codingllama pinging again for review. This repo appears to be configured to dismiss approvals for any changes made. |
Leveraging templates prevents dead code elimination for consumers of the trace library. The old method for generating the debug reports has been moved into trace_test.go and exposed via OldDebugReport functions so that the new handrolled reports can be validated against the templated version.