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

[otlp] Update OTel Collector PRW translate package to 0.73.0 #4063

Merged
merged 4 commits into from
Mar 27, 2023

Conversation

kovrus
Copy link
Contributor

@kovrus kovrus commented Jan 24, 2023

What this PR does

Updates the OTel Collector PRW translate package to 0.73.0. The PR contains the following user-facing changes:

  • Add support for converting OTLP Exponential Histograms to Prometheus Native Histograms.
  • Export _created metric for Summary, Histogram, and Monotonic Sum metric points if StartTimeUnixNano is set. It is disabled by default, the configuration parameter can be exposed in the future.
  • Do not drop exemplars of the OTLP Monotonic Sum metric.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2023

CLA assistant check
All committers have signed the CLA.

@kovrus kovrus changed the title [otlp] Update OTel Collector PRW translate dependency to 0.70.0 [otlp] Update OTel Collector PRW translate package to 0.70.0 Jan 24, 2023
@kovrus kovrus marked this pull request as ready for review January 24, 2023 11:35
@kovrus kovrus requested a review from a team as a code owner January 24, 2023 11:35
@kovrus kovrus requested a review from gouthamve January 24, 2023 11:36
@gouthamve
Copy link
Member

Can you update the end to end test to include an exponential histogram?

@pracucci
Copy link
Collaborator

Please also remember to sign the CLA. Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@kovrus kovrus force-pushed the otlp-dep-update branch 4 times, most recently from 5e9e76e to 367386c Compare February 17, 2023 13:22
@kovrus kovrus changed the title [otlp] Update OTel Collector PRW translate package to 0.70.0 [otlp] Update OTel Collector PRW translate package to 0.71.0 Feb 17, 2023
@kovrus
Copy link
Contributor Author

kovrus commented Feb 17, 2023

Can you please take a look at PR again? I've added a simple test case https://github.com/grafana/mimir/pull/4063/files#diff-57b597f7e1c988306aeac3c21175be693dd901c035761195212f24ca0a4a96b6R79 and updated the change log. (sorry, squashed the commits with addressing the feedback)

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I think I don't understand how this PR can adds supports to ingest exponential histograms as prometheus native histograms. Shouldn't we map them in promToMimirTimeseries() defined in pkg/util/push/otel.go?

CHANGELOG.md Outdated
Comment on lines 5 to 8
* [ENHANCEMENT] OTLP: Add support for converting OTel Exponential Histograms to Prometheus Native Histograms. #4063
* [BUGFIX] OTLP: Do not drop exemplars of the OTLP Monotonic Sum metric. #4063
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move these CHANGELOG entries below, under "### Grafana Mimir".

@pstibrany
Copy link
Member

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

@pracucci
Copy link
Collaborator

pracucci commented Mar 9, 2023

@kovrus Any update on this PR?

@kovrus
Copy link
Contributor Author

kovrus commented Mar 9, 2023

@pracucci didn't have a chance to work on it. I'll implement the missing mimir to prom ts conversion today or tomorrow.

@kovrus kovrus force-pushed the otlp-dep-update branch 2 times, most recently from 527af5c to 9c79819 Compare March 10, 2023 14:33
@kovrus
Copy link
Contributor Author

kovrus commented Mar 10, 2023

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

Under ## main / unreleased or ### Grafana Mimir?

@kovrus kovrus changed the title [otlp] Update OTel Collector PRW translate package to 0.71.0 [otlp] Update OTel Collector PRW translate package to 0.73.0 Mar 10, 2023
@kovrus kovrus requested a review from pracucci March 15, 2023 16:10
@kovrus
Copy link
Contributor Author

kovrus commented Mar 15, 2023

@pracucci @pstibrany can you take another look?

@kovrus kovrus force-pushed the otlp-dep-update branch 3 times, most recently from b83a3ca to d3a5bc6 Compare March 16, 2023 15:11
pkg/util/push/otel.go Show resolved Hide resolved
pkg/util/push/otel.go Outdated Show resolved Hide resolved
pkg/util/push/otel.go Show resolved Hide resolved
integration/otlp_ingestion_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/util/push/otel.go Outdated Show resolved Hide resolved
@kovrus kovrus requested a review from pstibrany March 17, 2023 12:35
@pstibrany
Copy link
Member

pstibrany commented Mar 20, 2023

kovrus requested a review from pstibrany

Just FYI: waiting for updated integration test that queries native histograms back after ingesting them.

@kovrus kovrus force-pushed the otlp-dep-update branch 8 times, most recently from a3978be to e52f8c7 Compare March 21, 2023 20:04
@kovrus
Copy link
Contributor Author

kovrus commented Mar 22, 2023

kovrus requested a review from pstibrany

Just FYI: waiting for updated integration test that queries native histograms back after ingesting them.

updated

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

pkg/util/push/otel.go Outdated Show resolved Hide resolved
It contains the following user facing changes:

- Add support for converting OTLP Exponential Histograms to Prometheus Native Histograms.
- Export `_created` metric for Summary, Histogram and Monotonic Sum metric points if StartTimeUnixNano is set.
  It is disabled by default, the configuration parameter can be exposed in future.
- Do not drop exemplars of the OTLP Monotonic Sum metric.
@pstibrany pstibrany merged commit 18b7f91 into main Mar 27, 2023
@pstibrany pstibrany deleted the otlp-dep-update branch March 27, 2023 10:24
ts.Exemplars = exemplars

return mimirpb.PreallocTimeseries{TimeSeries: ts}
}

func promToMimirHistogram(h *prompb.Histogram) mimirpb.Histogram {
pSpans := make([]mimirpb.BucketSpan, len(h.PositiveSpans))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The preallocation done here and below is incorrect. I opened a PR to fix it: #4639

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants