-
Notifications
You must be signed in to change notification settings - Fork 657
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
Conversation
2912c64
to
655f02d
Compare
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/__init__.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several questions
exporter/opentelemetry-exporter-prometheus/tests/test_mapping.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/tests/test_mapping.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/__init__.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Show resolved
Hide resolved
There was a problem hiding this 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 :)
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/_mapping.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!
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? |
Co-authored-by: Diego Hurtado <[email protected]>
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:
specification.
OTEL_PYTHON_EXPERIMENTAL_DISABLE_PROMETHEUS_UNIT_NORMALIZATION=true
s
->seconds
),following the collector's implementation
_
are replaced with a single_
{requests}
) are stripped awaym/s
->meters_per_second
.Type of change
Please delete options that are not relevant.
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?
Checklist: