-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[improve][broker] Reduce number of OpenTelemetry consumer attributes #22837
[improve][broker] Reduce number of OpenTelemetry consumer attributes #22837
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22837 +/- ##
============================================
- Coverage 73.57% 73.22% -0.35%
- Complexity 32624 32931 +307
============================================
Files 1877 1888 +11
Lines 139502 141756 +2254
Branches 15299 15558 +259
============================================
+ Hits 102638 103802 +1164
- Misses 28908 29936 +1028
- Partials 7956 8018 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Perfect @dragosvictor ! |
Motivation
PR #22693 introduced OpenTelemetry consumer stats. Two issues surfaced as part of a follow-up review:
Modifications
pulsar.consumer.connected_since
,pulsar.consumer.metadata
,pulsar.client.address
, andpulsar.client.version
.pulsar.consumer.blocked
from metric"pulsar.broker.consumer.message.unack.count
into its own metricpulsar.broker.consumer.blocked
.Verifying this change
This change added tests and can be verified as follows:
OpenTelemetryTopicStatsTest#testMessagingMetrics
to account for these changesDoes this pull request potentially affect one of the following parts:
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: dragosvictor#30