Skip to content
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

Merged
merged 7 commits into from
Dec 17, 2021

Conversation

keep94
Copy link
Contributor

@keep94 keep94 commented Nov 19, 2021

Description:
Add support for cumulative histograms

Testing:
Unit tests.

@keep94 keep94 requested review from a team and bogdandrutu November 19, 2021 19:32
@keep94 keep94 changed the title Add histogram consumer. [tanzuobservability exporter] Add histogram consumer. Nov 19, 2021
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thepeterstone thepeterstone left a 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

exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 30, 2021

CLA Signed

The committers are authorized under a signed CLA.

Use for..range where possible.
Sum up bucket counts for wavefront.
Make tests easier to read.
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics.go Outdated Show resolved Hide resolved
@jpkrohling jpkrohling changed the title [tanzuobservability exporter] Add histogram consumer. [exporter/tanzuobservability] Add histogram consumer. Dec 16, 2021
@jpkrohling jpkrohling changed the title [exporter/tanzuobservability] Add histogram consumer. [exporter/tanzuobservability] add histogram consumer Dec 16, 2021
@jpkrohling
Copy link
Member

If you want to work on the pending items on a follow-up PR, let me know and I'll merge this as is.

@keep94
Copy link
Contributor Author

keep94 commented Dec 16, 2021

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.

Copy link
Member

@jpkrohling jpkrohling left a 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.

@jpkrohling jpkrohling changed the title [exporter/tanzuobservability] add histogram consumer [exporter/tanzuobservability] Add histogram consumer Dec 17, 2021
@jpkrohling jpkrohling merged commit f5fecc7 into open-telemetry:main Dec 17, 2021
@keep94 keep94 deleted the histogram_metrics branch December 17, 2021 16:52
povilasv referenced this pull request in coralogix/opentelemetry-collector-contrib Dec 19, 2022
fixes #6389

Signed-off-by: Konstantin Demin <rockdrilla@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants