Add sanitizers for negative span duration #7122
Conversation
|
this issue is quite old issue, so i was not able to implement the solution the exactly as it was suggested, but i tried to fulfil the requirements from the issue as well as from the comments |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a sanitizer to correct negative span durations by setting them to 1 and annotating the span with a warning and tag. It also registers this sanitizer in the span processor options and adds unit tests to verify both positive and negative duration cases.
- Introduces
NegativeDurationSpanSanitizerand wires it intoBuildSpanProcessor - Appends warnings and tags for corrected negative durations
- Adds
TestNegativeDurationSpanSanitizerto cover zero, positive, and negative durations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/collector/app/span_handler_builder.go | Registered sanitizer option and implemented NegativeDurationSpanSanitizer method |
| cmd/collector/app/span_handler_builder_test.go | Added unit tests for negative-duration sanitization behavior |
Comments suppressed due to low confidence (1)
cmd/collector/app/span_handler_builder.go:94
- [nitpick] Consider renaming this method to
negativeDurationSpanSanitizer(lowercase) to keep it consistent with other unexported methods likemetricsFactoryanddefaultSpanFilter.
func (b *SpanHandlerBuilder) NegativeDurationSpanSanitizer(span *model.Span) *model.Span {
|
We don't need to wire this directly into the processor, we have a composable framework for sanitizers. |
f48f8b7 to
c71354f
Compare
|
i am not sure if i get it correctly, but as i see there is already |
74a1d73 to
731d9a8
Compare
9bf140d to
ae7a2a1
Compare
|
curiously, we had cmd/collector/app/sanitizer/zipkin/span_sanitizer.go which does this, but only for Zipkin format. I think the sanitizer is useful, but it does not address the referenced issue directly. It may help if the underlying cause for large numbers was negative duration, because the root cause of the issue seems to be the unsigned vs signed int representation in Jaeger's db struct vs ES data model. Arguably we have a bug in the ES code that should not be trying to use unsigned int and should take care of overflow before writing to ES. So let's rename the PR. And please add another sanitizer for v2 data model ./cmd/jaeger/internal/sanitizer/ |
b6c9468 to
d5c6eb0
Compare
|
will add the sanitizer for v2 data model tomorrow |
d2ac8dd to
e1a5398
Compare
14efa52 to
590ba4c
Compare
590ba4c to
e814f4d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7122 +/- ##
==========================================
- Coverage 96.20% 96.20% -0.01%
==========================================
Files 363 365 +2
Lines 21892 21940 +48
==========================================
+ Hits 21061 21107 +46
- Misses 621 623 +2
Partials 210 210
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…not match the struct field type uint64 . (jaegertracing#2517) Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
…r the one with new sanitizer Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
e40937a to
b1df1bb
Compare
a7c26c5
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test