-
Notifications
You must be signed in to change notification settings - Fork 409
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: Added consumer timeslice metrics from otel #2938
Conversation
6f8ee64
to
406028d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2938 +/- ##
==========================================
- Coverage 97.35% 97.30% -0.05%
==========================================
Files 317 317
Lines 48619 48648 +29
==========================================
+ Hits 47331 47338 +7
- Misses 1288 1310 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
few little tweaks, overall looks good
014739e
to
5027dc0
Compare
@@ -37,9 +38,11 @@ function createConsumerSegment(agent, otelSpan) { | |||
// 'host', | |||
// | |||
// ) | |||
transaction.name = segmentName | |||
transaction.setPartialName(segmentName) |
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.
you don't finalize the name here. That will be done in the onEnd
when we end the consumer span's transaction later. Also the segmentName
only needs to be ${system}/${destKind}/Named/${destination}
.
lib/otel/segments/consumer.js
Outdated
|
||
const segment = agent.tracer.createSegment({ | ||
recorder, | ||
name: segmentName, |
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.
based on the message-shim/subscribe-consume. this needs to be transaction.getFullName()
test/unit/lib/otel/consumer.test.js
Outdated
@@ -59,8 +59,8 @@ test('should create consumer segment from otel span', (t) => { | |||
const { segment, transaction } = synth.synthesize(span) | |||
assert.equal(segment.name, expectedName) | |||
assert.equal(segment.parentId, segment.root.id) | |||
assert.equal(transaction.name, expectedName) | |||
assert.equal(transaction.type, 'bg') | |||
assert.equal(transaction.name.endsWith(expectedName), 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.
this name should match exactly. the only thing you'll have to do before this, for now, is end the transaction. Once the reconciliation ticket is done(#2932) when the consumer span ends it will end the transsaction
const expectedMetrics = [ | ||
'OtherTransaction/all', | ||
'OtherTransaction/Message/all', | ||
'OtherTransaction/Message/OtherTransaction/Message/kafka/queue/Named/work-queue', |
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.
these names will need to be updated once you fix the suggestions in the consumer synthesizer. it shouldn't have OtherTransaction/Message
twice
7da113f
to
79b0927
Compare
This PR resolves #2929.