-
Notifications
You must be signed in to change notification settings - Fork 174
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
Define messaging metrics and add error.type
attribute to spans
#163
Conversation
cdbdf57
to
211bf76
Compare
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.
Thanks for putting a stake in the ground here, this is a great start.
11af3e7
to
4dcbade
Compare
6bdad37
to
fec893f
Compare
I love the duration metrics. Did we consider stronger consistency with existing
|
On the messaging side, the current metric names relate to the messaging specific operation names. For the consumer side I definitely see value in having separate consumer metrics pull-based (receive) and push-based (deliver) scenarios. The duration of pull and push durations aren't semantically consistent, as the push duration usually also involves the duration of processing the message, whereas the pull duration doesn't. We shouldn't mix both in one single metric. |
fec893f
to
2baa64d
Compare
2baa64d
to
034ff58
Compare
034ff58
to
d4aefb4
Compare
a1ec11b
to
adc9624
Compare
adc9624
to
6ae1dab
Compare
error.type
attribute to spans
For spans, messaging systems add system-specific attributes to the spans. Do we want to treat messaging system specific metric dimensions the same way, so that messaging systems extend existing metrics? Or do we require them to send different system-specific metrics? The first approach makes cardinality hard to control (imagining a single service using two different messaging systems), the second one will likely duplicate information. |
5b1cc90
to
93112d0
Compare
I think there a couple of options here:
I suggest to start with Option 1 - as messaging semconv progresses towards stability, we should get more feedback from messaging systems and instrumentation prototypes and can change the approach. |
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.
There are still some details to clarify, but this provides a great starting point.
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.
Overall looks like a good start! Left some non blocking comments.
93112d0
to
14c18bf
Compare
@open-telemetry/specs-semconv-approvers this PR is approved by the messaging WG members, please take a look |
Fixes open-telemetry/opentelemetry-specification#1014