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.

Sign up for GitHub

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

Jump to bottom

Modify Prometheus exporter to translate non-monotonic Sums into gauges #3306

Merged
merged 16 commits into from
Aug 7, 2023
Merged

Modify Prometheus exporter to translate non-monotonic Sums into gauges #3306

merged 16 commits into from
Aug 7, 2023

Conversation

brettimus
Copy link
Contributor

@brettimus brettimus commented May 10, 2023 •
edited
Loading

Description

This PR modifies the prometheus exporter code that translates OpenTelemetry metrics into Prometheus metrics.

Prometheus has the concept of a Gauge, which is effectively a non-monotonic Sum. (In the world of Prometheus, a monotonic Sum is called a Counter.)

At present, the code that translates OTEL metrics into Prometheus metrics currently treats all Sums as Prometheus "Counters", regardless of their monotonicity.

Quoting @dashpole in issue #3071

The prometheus compatibility spec for sums says: If the aggregation temporality is cumulative and the sum is non-monotonic, it MUST be converted to a Prometheus Gauge.

Therefore, I've modified the Prometheus exporter to always convert non-monotonic Sums (with cumulative aggregation temporality) into Prometheus Gauges.

Fixes #3071

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • tox -e opentelemetry-exporter-prometheus
  • tox -e lint

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8

  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md

  • Yes. - Link to PR:

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated (NOT RELEVANT)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 10, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@brettimus brettimus mentioned this pull request May 10, 2023
Closed
@brettimus brettimus force-pushed the export-nonmonotonic-sums-as-gauges-in-prometheus branch from 4777f46 to 01fc016 Compare May 10, 2023 16:28
@brettimus brettimus marked this pull request as ready for review May 10, 2023 16:29
@brettimus brettimus requested a review from a team as a code owner May 10, 2023 16:29
This was referenced May 10, 2023
Merged
Closed
@brettimus
Modify Prometheus exporter to translate non-monotonic Sums into gauges
ca4eb1f 
Add unit tests for translating monotonic and non-monotonic Sums to prometheus metrics

Update prometheus metric translation to match spec

Added a check for the aggregation temporality of a Sum.
"If the aggregation temporality is cumulative and the sum is non-monotonic, it MUST be converted to a Prometheus Gauge."

Update changelog
@brettimus brettimus force-pushed the export-nonmonotonic-sums-as-gauges-in-prometheus branch from 01fc016 to ca4eb1f Compare May 15, 2023 08:48
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 17, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@brettimus brettimus force-pushed the export-nonmonotonic-sums-as-gauges-in-prometheus branch from 64110c1 to 0ceeea2 Compare May 17, 2023 22:00
@brettimus
Copy link
Contributor Author

hey all! would it be possible to approve and/or merge this before the next release? i contribute to an open source lib that would benefit from this fix

thank you!!

@brettimus brettimus mentioned this pull request Jun 23, 2023
Closed
@brettimus
Copy link
Contributor Author

hi there! the only issue in the last run of the workflows had to do with linting afaik.

i just ran tox -e lint locally and everything passed. this PR should be ready to rerun the workflows.

(sorry for that hiccup by the way)

@srikanthccv srikanthccv requested a review from aabmass July 9, 2023 17:08
aabmass
aabmass approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, lgtm

@aabmass
Copy link
Member

aabmass commented Jul 10, 2023

@brettimus looks like there are build failures, could you take a look?

@brettimus
Copy link
Contributor Author

looks like i needed to update my new test after #3279 was merged

everything is green again locally! though i'm sad i didn't get to this sooner. i missed the release by a day!

@brettimus
Copy link
Contributor Author

Okay so everything is green except for the sdk tests for pypy3 on Windows.

The CI logs suggest that the error is unrelated to my change. Have any of you seen this before?

image

@srikanthccv srikanthccv added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jul 17, 2023
@brettimus
Copy link
Contributor Author

heck yeah! all green 🟢

@ocelotl ocelotl enabled auto-merge (squash) August 7, 2023 12:59
@ocelotl ocelotl merged commit c9277ff into open-telemetry:main Aug 7, 2023
111 checks passed
@dashpole dashpole mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

Assignees
No one assigned
Labels
PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prometheus exporter should convert non-monotonic sums to gauges
4 participants
@brettimus @aabmass @srikanthccv @ocelotl