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

receiver/prometheus: Fix start timestamp adjustment logic #3047

Merged
merged 9 commits into from
May 14, 2021

Conversation

Aneurysm9
Copy link
Member

@Aneurysm9 Aneurysm9 commented Apr 28, 2021

Description: The Prometheus receiver has some logic that attempts to detect resets in metric series and augments scraped metrics with start timestamps based on its understanding of when a monotonic metric was last reset. The old logic involved storing the previous value for a timeseries and calculating a delta from that value, but it resulted in dropping the initial samples received for counters, histograms, and summaries as well as understating values from those metrics by an amount equal to the initial sample. The adjustment logic has been changed to only adjust the start timestamp if required and not change any metric values.

Testing: No tests were added, existing unit and e2e tests were updated to reflect the correct expectations.

Documentation: The DESIGN.md document describing the receiver design was updated to reflect the modified start timestamp adjustment logic and for general copy editing.

Fixes open-telemetry/prometheus-interoperability-spec#40.
Fixes open-telemetry/prometheus-interoperability-spec#42.
Fixes open-telemetry/prometheus-interoperability-spec#45.

@Aneurysm9 Aneurysm9 requested a review from a team April 28, 2021 21:38
@Aneurysm9
Copy link
Member Author

@bogdandrutu
Copy link
Member

/cc @dashpole for GKE use-case

@dashpole
Copy link
Contributor

I think this is the right thing to do based on the metrics data model specification from my reading of it, but i'm fairly sure we will need to make some adjustment so it isn't rejected by the googlecloud exporter. See census-ecosystem/opencensus-go-exporter-stackdriver#266 as an example. GCM doesn't seem to accept points with identical start and end times.

@Aneurysm9
Copy link
Member Author

Should those adjustments be made in the export path, or is there a requirement in OTLP that start time and end time are not equal?

@dashpole
Copy link
Contributor

I don't think there is an OTLP requirement that they differ, but @bogdandrutu can correct me. When I say "we need to make an adjustment", I probably mean the stackdriver exporter needs to make an adjustment :)

@dashpole
Copy link
Contributor

dashpole commented May 5, 2021

The opentelemetry go exporter for GCM already does some adjustment, but I don't see it in the opencensus exporter for GCM.

@dashpole
Copy link
Contributor

dashpole commented May 5, 2021

I opened census-ecosystem/opencensus-go-exporter-stackdriver#287 to address this on stackdriver's end. I haven't tested it end-to-end, but I don't think it should block progress on this PR anymore.

Copy link
Contributor

@rakyll rakyll 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 good, just added a naming suggestion.

receiver/prometheusreceiver/DESIGN.md Outdated Show resolved Hide resolved
receiver/prometheusreceiver/internal/metrics_adjuster.go Outdated Show resolved Hide resolved
receiver/prometheusreceiver/internal/metrics_adjuster.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@Aneurysm9 can you resolve all the comments?

@Aneurysm9
Copy link
Member Author

@bogdandrutu I believe all comments have been resolved.

@bogdandrutu bogdandrutu merged commit 9f9e521 into open-telemetry:main May 14, 2021
Frefreak pushed a commit to Frefreak/opentelemetry-collector that referenced this pull request May 15, 2021
…etry#3047)

* receiver/prometheus: Do not drop metrics used to establish start time

Signed-off-by: Anthony J Mirabella <[email protected]>

* receiver/prometheus: update documentation regarding metric adjustment

Signed-off-by: Anthony J Mirabella <[email protected]>

* exporter/prometheus: fix e2e test to account for proper recevier functioning

* Addres PR feedback on docs and naming

Signed-off-by: Anthony J Mirabella <[email protected]>

* rename adjustPoint->isReset

Signed-off-by: Anthony J Mirabella <[email protected]>
dashpole pushed a commit to dashpole/opentelemetry-collector that referenced this pull request Jun 14, 2021
…etry#3047)

* receiver/prometheus: Do not drop metrics used to establish start time

Signed-off-by: Anthony J Mirabella <[email protected]>

* receiver/prometheus: update documentation regarding metric adjustment

Signed-off-by: Anthony J Mirabella <[email protected]>

* exporter/prometheus: fix e2e test to account for proper recevier functioning

* Addres PR feedback on docs and naming

Signed-off-by: Anthony J Mirabella <[email protected]>

* rename adjustPoint->isReset

Signed-off-by: Anthony J Mirabella <[email protected]>
ankitnayan pushed a commit to SigNoz/opentelemetry-collector that referenced this pull request Jul 27, 2021
…etry#3047)

* receiver/prometheus: Do not drop metrics used to establish start time

Signed-off-by: Anthony J Mirabella <[email protected]>

* receiver/prometheus: update documentation regarding metric adjustment

Signed-off-by: Anthony J Mirabella <[email protected]>

* exporter/prometheus: fix e2e test to account for proper recevier functioning

* Addres PR feedback on docs and naming

Signed-off-by: Anthony J Mirabella <[email protected]>

* rename adjustPoint->isReset

Signed-off-by: Anthony J Mirabella <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants