Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Pulsar Consumer metrics #11891
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
Add Pulsar Consumer metrics #11891
Changes from 32 commits
b954474
d855f97
b5d512b
3de71a8
6bb2c90
6f68ebd
9f42598
8b5e585
53b2dee
53b4471
df187f4
e2d5e09
8651722
52405fd
8bfc6fd
61d402d
fde08c4
7991bef
3ca21a9
e889e16
2c7b92e
9710b0d
84c9fbe
a0d0cd8
6780c4d
dfc799c
fe04048
c849a3b
8573d0e
8bb50f5
99c8256
807d60b
06e8b8e
76b103d
cd83eed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should we go ahead and update to the latest spec to avoid additional churn? https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-metrics.md
cc @open-telemetry/semconv-messaging-approvers
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
semConvVersion
hasn't been upgraded to the latest version yet. Is it okay to only upgrade some semantics?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.
yes it's ok, if we don't have the new constants available yet, you can create static final constants in this class and reference those
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 think this isn't quite correct. We should record the count of all messages but currently we are recording only the count of batch messages. Perhaps
getReceiveMessagesCount
should returnlong
instead ofLong
and whenMESSAGING_BATCH_MESSAGE_COUNT
attribute is missing then return1
.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 change will cause the
messaging.receive.messages
metric to appear in regular consumption as well, which does not align with the description in the spec.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.
Imo you are misinterpreting the spec. It says that this metric is not required when messaging system does not support batch receives because the receive count can be obtained from a different metric. The metric description says
Measures the number of received messages.
which in my opinion means all messages not only messages that were received as a batch. Another question is since this metric is opt-in shouldMessagingConsumerMetrics.get()
take a boolean argument that could be used to opt-in to this metric when instrumentation supports batch receives? @trask do you have any suggestions for thisThere 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.
Thank you for your addition.
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.
let's align with the latest semconv which I think answer this question?