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

Fix prometheus metric name and unit conversion #3924

Merged
merged 7 commits into from
May 28, 2024

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented May 22, 2024

Important

This is a breaking change. This PR changes the output prometheus metric names, but the API is not changed.

Description

Fixes #2938

Updates the prometheus exporter name conversion to match the specification by implementing a few fixes:

  • this is a breaking change to prometheus metric names so they comply with the
    specification.
  • you can temporarily opt-out of the unit normalization by setting the environment variable
    OTEL_PYTHON_EXPERIMENTAL_DISABLE_PROMETHEUS_UNIT_NORMALIZATION=true
  • common unit abbreviations are converted to Prometheus conventions (s -> seconds),
    following the collector's implementation
  • consecutive _ are replaced with a single _
  • unit annotations (enclosed in curly braces like {requests}) are stripped away
  • units with slash are converted e.g. m/s -> meters_per_second.
  • The exporter's API is not changed

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

I added a ton of test cases. Many were copied from the collector's test cases. I also added tests for semconv metrics so we can be sure what they will look like.

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

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

@aabmass aabmass requested a review from a team as a code owner May 22, 2024 17:05
@aabmass aabmass requested a review from dashpole May 22, 2024 17:06
@aabmass aabmass force-pushed the prom-unit-2938 branch 2 times, most recently from 2912c64 to 655f02d Compare May 22, 2024 18:27
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Several questions

Copy link
Member Author

@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.

Appreciate the quick review :)

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

My comments have been addressed thanks!

@jeremydvoss
Copy link
Contributor

This PR appears to be facing the same spelling issue as mine. Every "assertIn" is now marked as incorrect spelling. How do we add words to the dictionary. Is it just the ignore list in .codespellrc?

@lzchen
Copy link
Contributor

lzchen commented May 28, 2024

This PR appears to be facing the same spelling issue as mine. Every "assertIn" is now marked as incorrect spelling. How do we add words to the dictionary. Is it just the ignore list in .codespellrc?

#3930

@ocelotl ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label May 28, 2024
@ocelotl ocelotl enabled auto-merge (squash) May 28, 2024 18:33
@ocelotl ocelotl merged commit d73593d into open-telemetry:main May 28, 2024
233 checks passed
@aabmass aabmass deleted the prom-unit-2938 branch May 28, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update prometheus metric exporter to comply with data model specification
6 participants