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

[tanzuobservability exporter] Add consumer for sum metrics. #6385

Merged
merged 4 commits into from
Dec 1, 2021

Conversation

keep94
Copy link
Contributor

@keep94 keep94 commented Nov 19, 2021

Description:
Add support for sum metrics.

Testing:
Unit tests.

@keep94 keep94 requested review from a team and mx-psi November 19, 2021 16:08
@keep94 keep94 changed the title Add consumer for sum metrics. [tanzuobservability exporter] Add consumer for sum metrics. Nov 19, 2021
return pdata.MetricDataTypeSum
}

func (s *sumConsumer) Consume(metric pdata.Metric, errs *[]error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification gives some indications on how Sums are mapped to a timeseries model (PRW-based), in which monotonic Sums are mapped as counters while non-monotonic sums are mapped as gauges.

What to do here ultimately depends on your timeseries model (which I don't know), so this implementation is not necessarily wrong, but I wanted to bring it up here just in case it helped

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. Tanzu observability has a special API call to send a delta sum metric. To send a cumulative sum metric to wavefront, you use the same API call that you would use to send a gauge metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. Tanzu observability has a special API call to send a delta sum metric. To send a cumulative sum metric to wavefront, you use the same API call that you would use to send a gauge metric.

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
pushGaugeSingleNumberDataPoint->pushGaugeNumberDataPoint
pushSingleNumberDataPoint->pushNumberDataPoint
Copy link
Member

@oppegard oppegard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small changes and then I think we're good!

exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved
exporter/tanzuobservabilityexporter/metrics_test.go Outdated Show resolved Hide resolved
Copy link
Member

@oppegard oppegard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Nov 30, 2021
@bogdandrutu bogdandrutu merged commit 4f24c27 into open-telemetry:main Dec 1, 2021
@keep94 keep94 deleted the sum_metrics branch December 3, 2021 17:04
jamesmoessis pushed a commit to atlassian-forks/opentelemetry-collector-contrib that referenced this pull request Dec 8, 2021
…emetry#6385)

* [tanzuobservability exporter] Add consumer for sum metrics.

* Function headers to be more readable.

pushGaugeSingleNumberDataPoint->pushGaugeNumberDataPoint
pushSingleNumberDataPoint->pushNumberDataPoint

* Break verifySumConsumer into four separate tests.

* Minor changes to tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants