-
Notifications
You must be signed in to change notification settings - Fork 579
feat(instrumentation-kafkajs): update semantic conventions #2752
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
feat(instrumentation-kafkajs): update semantic conventions #2752
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2752 +/- ##
==========================================
+ Coverage 89.20% 89.44% +0.24%
==========================================
Files 168 172 +4
Lines 8076 8310 +234
Branches 1547 1591 +44
==========================================
+ Hits 7204 7433 +229
- Misses 872 877 +5
🚀 New features to boost your workflow:
|
bca0213
to
e092462
Compare
spans.push( | ||
instrumentation._startConsumerSpan({ | ||
topic: payload.batch.topic, | ||
message, | ||
operationType: MESSAGING_OPERATION_TYPE_VALUE_PROCESS, | ||
link: origSpanLink, | ||
attributes: { | ||
[ATTR_MESSAGING_DESTINATION_PARTITION_ID]: String( | ||
payload.batch.partition | ||
), | ||
}, | ||
}) | ||
); |
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.
This isn't new code but it feels a little bit wrong when compared to the examples in the semantic conventions doc (this one, specifically). All of these process
spans for each message will have the same start/end times as the batch itself and I'm not sure that's helpful or intuitive. But for now, I'm leaving that design choice alone and just adding the corresponding metric that the semantic convention requires for all process
spans.
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 this might be out of scope for this PR. Think creating a single receive
span with messaging.batch.message_count
attribute would be fine and then add the message offset, parent span context as links, not as new spans. (Trying this out with SQS: #2345)
e092462
to
00e0787
Compare
…collection for testability
…n partition IDs a message with a partition ID of `0` should now have the proper attribute set
…arate method for testability
Which problem is this PR solving?
Semantic conventions used in
@opentelemetry/instrumentation-kafkajs
are quite outdated.Short description of the changes
@opentelemetry/semantic-conventions
in@opentelemetry/instrumentation-kafkajs
to version 1.30.0.