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

[prometheusreceiver/prometheusexporter] Handle units in prometheus metric names #8950

Closed
dashpole opened this issue Mar 29, 2022 · 12 comments
Closed

[prometheusreceiver/prometheusexporter] Handle units in prometheus metric names #8950

dashpole opened this issue Mar 29, 2022 · 12 comments
Labels
comp:prometheus Prometheus related issues spec:metrics

Comments

@dashpole
Copy link
Contributor

After open-telemetry/opentelemetry-specification#2440, prometheus exporters must add units to as a suffix to the metric name (before type suffixes), and the prometheus receiver must trim a unit suffix after trimming type suffixes. I.e.:

# TYPE request_duration_seconds counter
request_duration_seconds_total{}

is broken down into OTLP

{
  name: request_duration,
  unit: seconds, 
  type: counter,
  is_monotonic: true,
}
@dashpole dashpole added spec:metrics comp:prometheus Prometheus related issues labels Mar 29, 2022
@bertysentry
Copy link
Contributor

Is this ever going to be implemented? I think it's a very important feature to make sure metric naming conventions are followed on both OpenTelemetry and Prometheus.

But when this will be implemented, it will be a breaking change on the receiving end! (think of a Grafana dashboard relying on Otel metrics named after the Otel metrics convention, that would suddenly break once this is merged into the collector).

So, this is the kind of change that needs to be implemented as soon as possible, to break as few things as possible.

@dashpole
Copy link
Contributor Author

dashpole commented May 2, 2022

Yes, this will definitely be implemented eventually, and I agree with the urgency. It is just a matter of people having time to do the work. You can see the full set of prom-spec items here: https://github.com/open-telemetry/opentelemetry-collector-contrib/issues?q=is%3Aopen+label%3Acomp%3Aprometheus+label%3Aspec%3Ametrics+

@bertysentry
Copy link
Contributor

At some point, my company will need this feature in prometheusexporter so we'll probably give it a try. However, I'm concerned with the many edge cases where appending Otel metrics will have adverse effects (when the metric name already contains the unit, or when the unit is a long string, etc.)

@bertysentry bertysentry mentioned this issue May 15, 2022
Merged
@bertysentry
Copy link
Contributor

@dashpole If you have time to have a look at the #10028 PR (just describing the change, before going down to the implementation)

bertysentry added a commit to sentrysoftware/opentelemetry-collector-contrib that referenced this issue Jun 22, 2022
@bertysentry
Issue open-telemetry#8950 prometheusexporter: Automatic rename of …
1a1f221 
…metrics to follow Prometheus naming convention

* Updated **prometheusremotewriteexporter** to normalize metric names
* Moved common code to new module /pkg/translator/prometheus
* Added documentation
djaglowski added a commit that referenced this issue Jun 23, 2022
@bertysentry @djaglowski
prometheusexporter: Automatic normalization of metrics from Otel to P…
b828db4 
…rometheus naming convention (#10028)

* Issue #8950 `prometheusexporter`: Automatic rename of metrics to follow Prometheus naming convention

* Updated **prometheusremotewriteexporter** to normalize metric names
* Moved common code to new module /pkg/translator/prometheus
* Added documentation

Co-authored-by: Dan Jaglowski <[email protected]>
@bertysentry
Copy link
Contributor

Prometheus exporters have been modified to append the unit (and _total) is necessary.

Now, we only need to update prometheusreceiver to do the reverse normalization!

This was referenced Nov 1, 2022
Closed
Merged
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Nov 9, 2022
@dashpole dashpole removed the Stale label Nov 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Jan 9, 2023
@bertysentry
Copy link
Contributor

Is this still on?

@dashpole dashpole removed the Stale label Jan 9, 2023
@kovrus kovrus mentioned this issue Feb 10, 2023
Closed
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Mar 13, 2023
@kovrus kovrus removed the Stale label Mar 13, 2023
@ekarlso
Copy link

ekarlso commented May 12, 2023

Any reason why this isn't fix? It basically breaks a ton of dashboards and I'd really like to avoid recreating them :§

@dashpole dashpole mentioned this issue Jun 30, 2023
Open
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Jul 12, 2023
@dashpole dashpole removed the Stale label Jul 12, 2023
This was referenced Jul 13, 2023
Merged
Merged
dmitryax pushed a commit that referenced this issue Jul 17, 2023
@dashpole
prometheusreceiver: Add trim_metric_suffixes configuration option (#2…
5a975ab 
…4256)

**Description:**

Adds a new configuration option: `trim_metric_suffixes` to the
prometheus receiver. When set to true, suffixes will be trimmed from
metrics. This replaces use of
the`pkg.translator.prometheus.NormalizeName` feature-gate in the
prometheus receiver, but leaves it for exporters.

The first commit simplifies the usage of the feature-gate. The tests and
implementation were passing around an entire registry when it wasn't
necessary.

**Link to tracking Issue:**

Part of
#21743
Part of
#8950
dmitryax added a commit that referenced this issue Jul 18, 2023
@dashpole @dmitryax
prometheus exporters: Add add_metric_suffixes configuration option (#…
2bc9904 
…24260)

**Description:**
Add add_metric_suffixes configuration option, which can disable the
addition of type and unit suffixes.
This is backwards-compatible, since the default is true.

This is recommended by the specification for sum suffixes in
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#sums
and allowed in metadata

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#metric-metadata-1:

`Exporters SHOULD provide a configuration option to disable the addition
of _total suffixes.`

`The resulting unit SHOULD be added to the metric as OpenMetrics UNIT
metadata and as a suffix to the metric name unless the metric name
already contains the unit, or the unit MUST be omitted`

This deprecates the BuildPromCompliantName function in-favor of
BuildCompliantName, which includes the additional argument for
configuring suffixes.

**Link to tracking Issue:**

Fixes
#21743
Part of
#8950

---------

Co-authored-by: Dmitrii Anoshin <[email protected]>
@dashpole
Copy link
Contributor Author

dashpole commented Sep 1, 2023

Fixed by #25887

@dashpole dashpole closed this as completed Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
comp:prometheus Prometheus related issues spec:metrics
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ekarlso @dashpole @kovrus @bertysentry