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

Rename "Label" concept to "Attribute" for consistency with OpenTelemetry tracing #1113

Closed
jmacd opened this issue Oct 19, 2020 · 11 comments
Closed
Assignees
Labels
area:data-model For issues related to data model priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory

Comments

@jmacd
Copy link
Contributor

jmacd commented Oct 19, 2020

What are you trying to achieve?

The resolution from #948 is that "Label" should be renamed "Attribute" for consistency across OpenTelemetry. This has been discussed widely and there are no objections on the Metrics SIG to this renaming.

This renaming will introduce a breaking change for JSON-format exporters using OTLP v0.5, but will not introduce a binary-format incompatibility when this change is released as OTLP v0.6.

The specification will be updated to explain that although we use a single concept and term to describe attributes, sometimes we choose to limit their values to only strings. To address this across OpenTelemetry, we will specify how to handle the situation whenever requesting a string-valued attribute in a context where it would be prohibitively expensive to support. These cases are:

  1. A list-valued attribute used as a metric attribute or as a resource attribute exported through a legacy metricexporter
  2. A map-valued attribute used as a ... (same) ...

In both cases, I think a simple text-value replacement like "unrepresentable" would satisfy me.

Additional context.

Issue #948 contains a lengthy discussion of this topic. The terms "Attribute", "Label", "Tag", "Property", and "Association" are all very close in meaning, and we have come to agree that having more than one of these terms in use causes more trouble than it helps with.

@jmacd jmacd added spec:metrics Related to the specification/metrics directory release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 19, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Oct 20, 2020

Important note from @tigrannajaryan as we specify what it means to have "attributes" be encoded as strings.

The specification will be rewritten to state that although the attribute concept is universal, the exact representation of attributes varies by telemetry signal.

I think this can be a source of confusion. It probably depends on how exactly you define the 2 different "attributes" in the spec, in the API and the protocol, so it would be good to see some drafts of what your proposal looks like before we fully commit to it.

@bogdandrutu
Copy link
Member

I am not sure I understand this motivation. For me there are 2 different concepts that do not belong to any signal (which is a major part of my confusion):

  • Labels - are key-value pairs with only string values.
  • Attributes - are key-value pairs with a lot of possibilities for the values.

Once we define these concepts, different signals can use one of them (that best fit the requirements). I am not sure what is the confusion, and not sure that making metrics using Attributes but limit the possible values set is better.

What is the problem that we solve if we rename the metrics key-values with Attributes but we support only String values, I think that is way more confusing. We should not do this change just to have same name with different behaviors, so far we do have consistent behavior for every of these usages.

@tedsuo
Copy link
Contributor

tedsuo commented Nov 3, 2020

Next steps, suggested from from today's Spec meeting:

  • Define how to serialize all attribute types. This is especially important for baggage, if they are going to use attributes.
    • Are all values coerced into string? Or do we preserve the type?
  • Once we've determined how these conversions work, replace the restricted-to-string Label types with Attributes.
    • Provide guidance to implementors for how to coerce types for more complex metrics operations.

@Oberon00
Copy link
Member

Oberon00 commented Nov 3, 2020

We also briefly considered including TraceState in this discussion, but decided that since we couple it to W3C TraceState and it has quite a few special rules and because it will typically have values that consist of multiple sub-keys per vendor/entry (serialized as string) it does not make sense to allow non-string values there.

@tsloughter
Copy link
Member

Not completely sure what the remaining questions are but:

Can metrics just be defined to only have "String Attributes"? Then anything else is dropped if it isn't a string. This gets rid of the multiple names for things while makes clear only strings are allowed.

Or is the issue that dropping isn't an option and thus have to define string representations of everything? Maybe reopen as an issue titled "How to Convert Non-String Attribute Values as Strings" :)

@tedsuo
Copy link
Contributor

tedsuo commented Feb 3, 2021

@bogdandrutu want to reassign this to me? I'm getting to work on a string conversion OTEP.

@jmacd jmacd added the area:data-model For issues related to data model label Feb 4, 2021
@tedsuo
Copy link
Contributor

tedsuo commented Feb 12, 2021

At the SIG today, we are going to move forwards at this time by:

  • Replacing Labels with Attributes at the API level
  • Have a conversion path for attributes to strings (at minimum for the Prometheus exporter)
  • No decision yet about how to approach metric attribute types for OTLP (string vs multi-value)

@reyang reyang self-assigned this Mar 3, 2021
@jsuereth
Copy link
Contributor

jsuereth commented Mar 16, 2021

We discussed this heavily today at the Metrics Data Model SiG. The three proposals discussed are here.

I'm including the "consensus proposal" of that group here for implementation (volunteered by @bogdandrutu ):

Use Attribute and treat different types values as different things.

In this proposal, we alter OTLP to leverage the same KeyValue proto on metrics and drop the use of Label / StringKeyValue in the specification. The API/SDKs will export metric "Attributes" that have string keys and multi-typed values (can be anything supported by AnyValue in OTLP). The collector will not attempt to unify types between attributes of the same name.

If two attribute keys on the "same metric" are received that have different types, this is considered an error.

  • Both data points will be dropped, with a warning to the user.
  • We expect to improve on this specification over-time in a backwards-compatible way (i.e. allowing more use-cases vs. issuing errors is considered a non-breaking change).

Note: Metric identity specification may need more clarity in the (pending) data model specification.

Pros

  • Consistency w/ Trace / Resource
  • Resource attributes can directly be mapped into metric attributes
  • Backends can leverage types in labels
  • Clear aggregation semantics.

Cons

  • More complicated OTLP handling (name+type vs. just name)
  • Error messages for conflicting types on labels could be delayed + confusing.
  • Prometheus/OM would end up specifying the toString method for attribute values anyway, leading to (possibly) disjoint handling OR a new specification on toString anyway. (Note: This is considered fine for OLTP data model, and we can/will work on this as part of Prometheus/OpenMetric compatibility).

@hdost
Copy link

hdost commented Apr 15, 2021

I wasn't able to make it to the Model SIG, what was actual blocker here?

@jsuereth
Copy link
Contributor

Give opentelemetry-proto#283 is merged here, I think remaining work is in the metrics API specification. Does that sound right @reyang ?

@reyang reyang removed this from the Metrics API/SDK Experimental Release milestone Apr 23, 2021
@reyang
Copy link
Member

reyang commented Apr 23, 2021

The API spec is also done (addressed by #1557).
I'm closing this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model For issues related to data model priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

No branches or pull requests

9 participants