-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v2][adjuster] Implement adjuster for sorting attributes and events #6389
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
for k := 0; k < spans.Len(); k++ { | ||
span := spans.At(k) | ||
s.sortAttributes(span.Attributes()) | ||
s.sortEvents(span.Events()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro Wanted a sanity check here. In v1, the "event" field was moved to the first position (ref: https://github.com/jaegertracing/jaeger/blob/main/model/adjuster/sort_tags_and_log_fields.go#L31). However, in v2, logs are stored as event attributes and the "event" attribute is stored as the event name (https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/jaeger/jaegerproto_to_traces.go#L406).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm. Good to have first class fields.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6389 +/- ##
==========================================
- Coverage 96.22% 96.19% -0.03%
==========================================
Files 361 362 +1
Lines 20621 20671 +50
==========================================
+ Hits 19843 19885 +42
- Misses 595 601 +6
- Partials 183 185 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mahad Zaryab <[email protected]>
s.sortAttributes(resource.Attributes()) | ||
scopeSpans := rs.ScopeSpans() | ||
for j := 0; j < scopeSpans.Len(); j++ { | ||
ss := scopeSpans.At(j) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not sort scope attributes?
ss := scopeSpans.At(j) | |
ss := scopeSpans.At(j) | |
s.sortAttributes(ss.Attributes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro Done. And did the same for link attributes as well. We should have all collections accounted for now which should provide us with a way to deterministic hash the traces.
Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
SortTagsAndLogFields
adjuster in v1.How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test