-
Notifications
You must be signed in to change notification settings - Fork 26
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
Centralize retrieving instrumentation scope framework conversion for all signals #339
Centralize retrieving instrumentation scope framework conversion for all signals #339
Conversation
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! Can we add a test for metrics and logs (similar to
apm-data/input/otlp/traces_test.go
Lines 567 to 579 in 2089180
func TestInstrumentationLibrary(t *testing.T) { | |
traces, spans := newTracesSpans() | |
spans.Scope().SetName("library-name") | |
spans.Scope().SetVersion("1.2.3") | |
otelSpan := spans.Spans().AppendEmpty() | |
otelSpan.SetTraceID(pcommon.TraceID{1}) | |
otelSpan.SetSpanID(pcommon.SpanID{2}) | |
events := transformTraces(t, traces) | |
event := (*events)[0] | |
assert.Equal(t, "library-name", event.Service.Framework.Name) | |
assert.Equal(t, "1.2.3", event.Service.Framework.Version) | |
} |
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.
The PR is missing a description and a link to the issue it fixes.
I think metrics is already covered in the tests that I updated here: Added an additional test for logs @lahsivjar |
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! Thanks for adding the tests.
@lahsivjar - When will this enhancement become available for customers? Is it only applicable to MIS? Thanks |
@simitt - Wondering if you can help me with the timelines/release for availability of this enhancement! Thanks |
Hi @akhileshpok , apologies, I missed your previous message. @lahsivjar could you cut a release for this and ensure that automation picks up the new version? |
@akhileshpok Apologies about missing the previous ping, it fell through the cracks 😓
Will do! |
@akhileshpok @simitt Here is the PR for adding the fix to APM-Server. It will be merged soon and should be available with |
Addresses: #325
This PR ensures we map OpenTelemetry Instrumentation Scope to
Service.Framework.*
for all signal types.