-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[exporter/tanzuobservability] Add histogram consumer #6389
Conversation
3b2bb9b
to
b058873
Compare
2f59e26
to
e38afa6
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.
A few minor comments, otherwise LGTM
Use for..range where possible. Sum up bucket counts for wavefront. Make tests easier to read.
5e244f0
to
58fbd32
Compare
58fbd32
to
54c4dc7
Compare
If you want to work on the pending items on a follow-up PR, let me know and I'll merge this as is. |
I believe I have addressed all pending comments. Would you please have another look? I decided to control whether internal metrics get sent from the top level metricConsumer type rather than from each typedMetricConsumer. Now, typedMetricConsumer.PushInternalMetrics() always sends internal metrics if called. A bool flag controls whether the top level metricConsumer calls PushInternalMetrics() on each typedMetricConsumer. |
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 looks OK to me. Please, do consider reworking the internal metrics, so that observability for the component itself is done in line with other components.
fixes #6389 Signed-off-by: Konstantin Demin <rockdrilla@gmail.com>
Description:
Add support for cumulative histograms
Testing:
Unit tests.