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

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for Histogram OTLP metrics #1643

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

huyan0
Copy link
Member

@huyan0 huyan0 commented Aug 27, 2020

This PR is part of a series of PRs implementing a Prometheus remote write exporter supporting Cortex.
See related PR #1577

Description: This PR adds export support for histogram and summary metrics for Prometheus remote write integrated backends, such as Cortex. The exporter drops non-cumulative monotonic, histogram, and summary OTLP metrics.

Please note this metrics exporter does not support Prometheus default labels such as job or instance labels. An issue addressing Prometheus default labels will be filed later. Another related feature is to derive labels from a Resource. This functionality already exists in the Go SDK and will be implemented in another PR. This feature could allow users to specify which attributes they want to add as labels.

Link to tracking Issue: #1150
Related issues are:
Metrics aggregation proposal: #1422
Prometheus exporter not functional: #1255
Related spec discussion: #731

Documentation:

cc: @huyan0 @alolita @jmacd @bogdandrutu

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #1643 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1643      +/-   ##
==========================================
+ Coverage   92.37%   92.40%   +0.03%     
==========================================
  Files         265      265              
  Lines       19909    19952      +43     
==========================================
+ Hits        18390    18437      +47     
+ Misses       1090     1088       -2     
+ Partials      429      427       -2     
Impacted Files Coverage Δ
exporter/prometheusremotewriteexporter/helper.go 100.00% <ø> (ø)
exporter/prometheusremotewriteexporter/exporter.go 95.94% <100.00%> (+5.37%) ⬆️
service/defaultcomponents/defaults.go 85.45% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09d5ee7...ebb3f11. Read the comment docs.

@bogdandrutu
Copy link
Member

Please split this into 2 PRs the first being the Histogram. We will not support Summary for few weeks in the new otlp and that will get delayed.

@huyan0 huyan0 changed the title Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for Histogram and Summary OTLP metrics Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for Histogram OTLP metrics Aug 27, 2020
@bogdandrutu
Copy link
Member

Please increase the coverage :)

@huyan0
Copy link
Member Author

huyan0 commented Aug 28, 2020

Please increase the coverage :)

Will do. Also rebasing : )

@huyan0 huyan0 force-pushed the prw_exporter branch 2 times, most recently from 7b952a7 to 73f05c5 Compare August 28, 2020 17:34
@huyan0 huyan0 marked this pull request as ready for review August 28, 2020 18:30
@huyan0
Copy link
Member Author

huyan0 commented Aug 28, 2020

Got 100% diff coverage now

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

A lot of unnecessary changes can you please fix them?

exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved
@huyan0
Copy link
Member Author

huyan0 commented Aug 31, 2020

Addressed some comments. Really appreciate the review. Will start working on #1681

@huyan0 huyan0 requested a review from bogdandrutu August 31, 2020 13:50
@huyan0 huyan0 requested a review from bogdandrutu August 31, 2020 16:10
@huyan0 huyan0 requested a review from ercl August 31, 2020 19:42
@vishiy
Copy link

vishiy commented Mar 2, 2021

@huyan0 @bogdandrutu @EdZou @ercl - I see the prometheus histograms represented as OTEL histograms, during which the values are becoming delta ( see design here - https://github.com/open-telemetry/opentelemetry-collector/blob/main/receiver/prometheusreceiver/DESIGN.md#histogram ). I see the same issue with summary (for _sum & _count) and also counters. Is there a way to configure the receiver to NOT do the delta/diff from first scrape ? If not, how to get the original values as scraped for these metrics to be able to ingest to prometheus thru the prometheus exporter?

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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