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

Add Instrumentation Scope and Version as labels in Prometheus #2703

Merged
merged 6 commits into from Oct 12, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jul 29, 2022

Fixes: #2493

Related: #1906

This is a second attempt at #2422.

No longer depends on #2702

Changes

Background: Naming Collisions

OpenTelemetry encourages the use of semantic conventions to make metric naming similar across instrumentation. For example, if I have two http client libraries in my application, they would each produce a metric named http.client.duration, but with different meters (e.g. otelmux vs otelhttp). A prometheus exporter which receives both of these metrics would not be able to serve both of those histograms. This would occur anytime a user uses two libraries which produces the same category (e.g. http, database, rpc, etc) of metrics, or if the two libraries just happen to use the same name for a metric. Depending on the language, it may fail to create the Prometheus exporter, or may fail to send some, or all metrics if the same labels keys and values are present in both.

Desired User Experience

As a user, I can use a Prometheus exporter with OpenTelemetry without experiencing strange errors/behavior due to naming collisions, and without having to apply transformations to metric names to work around these, except in rare cases.

As a user, I can easily add scope attributes to my metrics in Prometheus by joining with an info-style metric. This is a common pattern in Prometheus: https://grafana.com/blog/2021/08/04/how-to-use-promql-joins-for-more-effective-queries-of-prometheus-metrics-at-scale/.

Design

Add opentelemetry_scope_name and opentelemetry_scope_version as labels to all metrics. This ensures that if two libraries produce the same metric points, they don't collide because the scope name/version labels will differ.

Those labels also serve as "join keys" to be able to add scope attributes to Prometheus metrics. This is accomplished by introducing an opentelemetry_scope_info metric containing the same opentelemetry_scope_name and opentelemetry_scope_version labels, but also including scope attributes. This also enables the collector's Prometheus receiver to reconstruct the original Instrumentation Scope when receiving the metrics.

@open-telemetry/wg-prometheus
@jmacd @tigrannajaryan @jsuereth

specification/metrics/data-model.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

dashpole commented Aug 3, 2022

A comment from the prometheus wg: We should consider splitting build_info out of target_info. There is a question about consistency across languages, and what the structure/overlap of those conventions are with otel's conventions.

That is orthogonal to this PR, though.

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels Aug 5, 2022
@dashpole dashpole changed the title Use scope.short_name to support instrumentation scope in Prometheus Use short_name to support instrumentation scope in Prometheus Aug 10, 2022
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 18, 2022
specification/metrics/data-model.md Outdated Show resolved Hide resolved
specification/metrics/data-model.md Outdated Show resolved Hide resolved
specification/metrics/data-model.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

The two ideas from the spec sig today were:

  1. Use the last word of the instrumentation scope name (and also, maybe allow overriding in the exporter w/ a mapping)
  2. Change guidance for the scope name to be shorter and readable. Deal with scope uniqueness separately.

The preference at the meeting seemed to be for the first option, so i'm going to update this PR to propose that.

@github-actions github-actions bot removed the Stale label Aug 31, 2022
@dashpole dashpole changed the title Use short_name to support instrumentation scope in Prometheus Use instrumentation scope as prefix in Prometheus Aug 31, 2022
@dashpole
Copy link
Contributor Author

I've updated the description and proposal to propose using the last word of the instrumentation scope.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Aug 31, 2022

  1. Use the last word of the instrumentation scope name (and also, maybe allow overriding in the exporter w/ a mapping)

I like this. If we allow overriding it in the exporter we have a complete solution. Name clashes are going to be rare and for that rare case requiring overriding in the exporter is fine. It can even be done via an env variable specific to prometheus exporter so that no code changes are necessary. We may want to have some diagnostics to detect the clashes and show in the logs or otherwise surface the problem.

Edit: based on examples @arminru and @joaopgrassi posted in the comment thread below, I am not so sure anymore that name clashes will be rare. So, maybe it is not such a good idea after all.

@dashpole
Copy link
Contributor Author

I believe @ahayworth's feedback, which was that we should not recommend adding a prefix to all metrics, has been addressed.

I'm happy with the current state, and would like to see this merged unless others have feedback.

@tigrannajaryan
Copy link
Member

@open-telemetry/specs-metrics-approvers @open-telemetry/technical-committee I will merge this tomorrow unless I see objections.

@ahayworth
Copy link
Contributor

@carlosalberto Thank you for poking me: I agree with @dashpole that my feedback has indeed been addressed. ❤️

CHANGELOG.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Changelog updated. No new changes for several days. No objections from anyone. Merging.

@tigrannajaryan tigrannajaryan merged commit a3ba53f into open-telemetry:main Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Record instrumentation_scope attribute in Prometheus export