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

opentelemetry: support attributes for metrics #2354

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aluminous
Copy link

Motivation

As detailed by @nmoutschen in #2337, attributes are currently not supported when recording metrics. Attributes are needed to add dimensions to metric data without enumerating all possible combinations as different metrics.

The linked issue contains more details regarding the motivation for this change.

Solution

This implements the solution suggested by @nmoutschen, where a special prefix of attribute. is used to define attributes to record with the metric. For example, the below records two counts for the metric my_counter_metric each with separate values for my_attribute.

info!(
    counter.my_counter_metric = 1,
    attribute.my_attribute = "first",
    attribute.is_example = true,
);

info!(
    counter.my_counter_metric = 1,
    attribute.my_attribute = "second",
    attribute.is_example = true,
);

Attributes may be i64, f64, str or bool as supported by opentelemetry::Value.

Fixes #2337

Add support for specifying attributes to be associated with metrics
using the prefix `attribute.`. This allow providing context or
dimensions for a given metric without changing the metric name.
@jtescher
Copy link
Collaborator

This seems like a reasonable addition to me, @hawkw do you have recommendations for naming conventions here? e.g. some of the layer/subscriber-specific keys are otel. prefixed and these metric ones are not. Seems like the advantages of not prefixing is that log output for the keys is less noisy, but the disadvantage is they are more ambiguous.

@hawkw
Copy link
Member

hawkw commented Oct 24, 2022

i think that if we expect the fields to be consumed exclusively by OpenTelemetry, they should probably be prefixed. unfortunately, i probably won't be able to actually look closely at this PR today (or probably this week) as i'm traveling to KubeCon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for attributes in tracing-opentelemetry::MetricsLayer
3 participants