chore: give hint of configuration on otel invalid utf8 errors#2551
chore: give hint of configuration on otel invalid utf8 errors#2551
Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an OTLP tracing error helper to detect invalid UTF-8 errors, integrates it into the meter error handler, and adds a test exercising logging of a sanitization hint when an exporter returns an invalid-UTF8 error. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
SkArchon
changed the title
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (14.28%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2551 +/- ##
===========================================
- Coverage 62.77% 25.61% -37.17%
===========================================
Files 242 243 +1
Lines 25560 25571 +11
===========================================
- Hits 16046 6549 -9497
- Misses 8178 18405 +10227
+ Partials 1336 617 -719
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/trace/meter.go (1)
218-222:⚠️ Potential issue | 🟡 Minor
otel.SetErrorHandlershould respect the same test-mode guard asotel.SetTracerProvider.The comment at Line 217 says "Don't set it globally when we use the router in tests," and
SetTracerProvideris correctly guarded byif config.MemoryExporter == nil. ButSetErrorHandleron Line 222 is unconditional and always mutates global state, even in test mode.In parallel test runs every call to
NewTracerProviderraces to replace the one global handler; the last writer wins. The new test atattribute_processor_test.go:191specifically relies on errors from itsinvalidUTF8Exporterreaching its own observer — if another parallel test overwrites the handler between setup and span export the assertion can fail non-deterministically.🛡️ Proposed fix: apply the same guard
- otel.SetErrorHandler(otel.ErrorHandlerFunc(errHandler(config))) + if config.MemoryExporter == nil { + otel.SetErrorHandler(otel.ErrorHandlerFunc(errHandler(config))) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/trace/meter.go` around lines 218 - 222, The global error handler is being set unconditionally causing test-time races; guard the otel.SetErrorHandler call the same way otel.SetTracerProvider is guarded: only call otel.SetErrorHandler(otel.ErrorHandlerFunc(errHandler(config))) when config.MemoryExporter == nil (i.e., inside the same if block that sets the tracer provider) so NewTracerProvider does not mutate global state during tests and the errHandler/config.MemoryExporter logic remains consistent.
🧹 Nitpick comments (2)
router/pkg/trace/errors.go (1)
8-22:invalidUTF8Errorinterface approach is sound — worth documenting the coupling.The
interface{ InvalidUTF8() bool }check matches the detection pattern the protobuf team themselves recommend (per the public issue tracker). However, sinceerrInvalidUTF8lives ingoogle.golang.org/protobuf/internal/impl, there's no stability guarantee — the type or method could be removed in a future protobuf release, causinghasInvalidUTF8Errorto silently returnfalsewithout any compile-time signal. The degradation is graceful (errors still log as plain"otel error"), but a brief comment noting this dependency would help future maintainers.
errors.Aswith a*invalidUTF8Error(pointer-to-interface) target is valid; the Go stdlib explicitly supports passing a pointer to any interface type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/trace/errors.go` around lines 8 - 22, Add a short comment above the invalidUTF8Error type and hasInvalidUTF8Error function that documents the coupling to google.golang.org/protobuf/internal/impl (errInvalidUTF8) — state that this check relies on the presence of an internal type exposing InvalidUTF8(), that the internal package is not guaranteed stable and could be removed or changed in future protobuf releases, and that in such cases hasInvalidUTF8Error will gracefully return false; reference the symbols invalidUTF8Error and hasInvalidUTF8Error and mention that errors.As is intentionally used with an interface target to detect that condition.router-tests/telemetry/attribute_processor_test.go (1)
206-212: Redundant double log filtering.
FilterMessageSnippet("sanitize_utf8").All()is called once insiderequire.Eventually(where the result is discarded) and then again on Lines 211–212. Afterrequire.Eventuallyexits successfully the log entries are guaranteed to exist; the second call is unnecessary.♻️ Proposed refactor
- require.Eventually(t, func() bool { - logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() - return len(logs) > 0 - }, 5*time.Second, 100*time.Millisecond) - - logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() - require.NotEmpty(t, logs) + var logs []observer.LoggedEntry + require.Eventually(t, func() bool { + logs = xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() + return len(logs) > 0 + }, 5*time.Second, 100*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/telemetry/attribute_processor_test.go` around lines 206 - 212, The test is calling xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() twice (once inside require.Eventually and again after), which is redundant; declare a local variable logs before the require.Eventually call, assign to logs inside the closure used by require.Eventually (e.g., logs = xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All(); return len(logs) > 0), and then after require.Eventually use require.NotEmpty(t, logs) so the log slice is only retrieved once; references: require.Eventually, xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All(), require.NotEmpty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Line 213: The test calls require.Equal with the actual value first and the
expected string second, which reverses Testify's expected ordering and produces
confusing failure messages; change the call to require.Equal(t, "otel error:
traces export: string field contains invalid UTF-8: Enable
'telemetry.tracing.sanitize_utf8.enabled' in your config to sanitize invalid
UTF-8 attributes.", logs[0].Message) so the literal expectation is the
second-to-last argument and logs[0].Message is the actual (last) argument;
update the invocation that references require.Equal and logs[0].Message
accordingly.
---
Outside diff comments:
In `@router/pkg/trace/meter.go`:
- Around line 218-222: The global error handler is being set unconditionally
causing test-time races; guard the otel.SetErrorHandler call the same way
otel.SetTracerProvider is guarded: only call
otel.SetErrorHandler(otel.ErrorHandlerFunc(errHandler(config))) when
config.MemoryExporter == nil (i.e., inside the same if block that sets the
tracer provider) so NewTracerProvider does not mutate global state during tests
and the errHandler/config.MemoryExporter logic remains consistent.
---
Nitpick comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Around line 206-212: The test is calling
xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() twice (once inside
require.Eventually and again after), which is redundant; declare a local
variable logs before the require.Eventually call, assign to logs inside the
closure used by require.Eventually (e.g., logs =
xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All(); return len(logs) >
0), and then after require.Eventually use require.NotEmpty(t, logs) so the log
slice is only retrieved once; references: require.Eventually,
xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All(), require.NotEmpty.
In `@router/pkg/trace/errors.go`:
- Around line 8-22: Add a short comment above the invalidUTF8Error type and
hasInvalidUTF8Error function that documents the coupling to
google.golang.org/protobuf/internal/impl (errInvalidUTF8) — state that this
check relies on the presence of an internal type exposing InvalidUTF8(), that
the internal package is not guaranteed stable and could be removed or changed in
future protobuf releases, and that in such cases hasInvalidUTF8Error will
gracefully return false; reference the symbols invalidUTF8Error and
hasInvalidUTF8Error and mention that errors.As is intentionally used with an
interface target to detect that condition.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router-tests/telemetry/attribute_processor_test.gorouter/pkg/trace/errors.gorouter/pkg/trace/meter.go
|
Is there a downside to turning it on by default? Seems like if it inevitably just makes errors we should do this implicitly. |
@SkArchon is validating the overhead. If negligible, I'd also suggest enabling it by default. |
|
It takes about |
3 participants
Right now we have options to sanitize span attributes, however this is off by default. Users right now will get the error message of "string field contains invalid UTF-8" when non utf8 data is set into a span anywhere. However users might not know that a workaround exists (right now for span attributes), without logging the error as is, if the error is related to non utf8 data we will log the error with a hint.
Summary by CodeRabbit
Bug Fixes
Tests
Checklist