-
Notifications
You must be signed in to change notification settings - Fork 16
[enrichments] Add support for inferred spans #116
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
Conversation
| spanLink.Attributes().RemoveIf(func(k string, v pcommon.Value) bool { | ||
| switch k { | ||
| case "is_child", "elastic.is_child": | ||
| if v.Bool() && !spanID.IsEmpty() { |
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.
[For reviewers] In APM data we add an empty string in the slice if span ID is empty (ref), however, I didn't think it was important so didn't handle it here. Let me know if it needs to be added.
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.
Yeah, that was not intentionally done in apm-data for that code path. I don't think span links are allowed to have an empty span ID in OTLP?
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.
I don't think span links are allowed to have an empty span ID in OTLP?
I am not sure about this, I kept the span ID check for now, let me know if you think we should remove this.
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.
In APM data we add an empty string in the slice if span ID is empty (ref), however, I didn't think it was important so didn't handle it here
Just wanted to agree with you here. I'd keep the span ID check as is just as a safety measure.
| spanLink.Attributes().RemoveIf(func(k string, v pcommon.Value) bool { | ||
| switch k { | ||
| case "is_child", "elastic.is_child": | ||
| if v.Bool() && !spanID.IsEmpty() { |
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.
Yeah, that was not intentionally done in apm-data for that code path. I don't think span links are allowed to have an empty span ID in OTLP?
| if tc.expectedSpanLinks != nil { | ||
| tc.expectedSpanLinks.CopyTo(expectedSpan.Links()) | ||
| } else { | ||
| expectedSpan.Links().RemoveIf(func(_ ptrace.SpanLink) bool { return true }) |
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.
[For reviewers] This piece is a bit weird because CopyTo doesn't handle non initialized empty span links.
JonasKunz
left a comment
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.
LGTM
Adds support for inferred spans, check agent specs for more details.