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

Consistent allowed attribute types #446

Closed
Oberon00 opened this issue Feb 6, 2020 · 10 comments · Fixed by #722
Closed

Consistent allowed attribute types #446

Oberon00 opened this issue Feb 6, 2020 · 10 comments · Fixed by #722
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory

Comments

@Oberon00
Copy link
Member

Oberon00 commented Feb 6, 2020

We currently have key-value pairs at different places:

  1. Span attributes
  2. Span link attributes
  3. Span event attributes
  4. Resource attributes
  5. Metric labels

2 and 3 reference 1 (although IMHO that could be spelled out more clearly) and are thus always in sync. Resources currently have the following differences:

IMHO, we should change the spec here to also reference Span's definition of attributes (first use case for arrays that comes to mind: List of a host's IP addresses or host names).

I couldn't find any specification at all on what a metric label value can be on master (but haven't looked in any PR). Clearly, there needs to be some specification here (probably not allowing array values).

Related: #76 "API: Consistent tag/attribute/field/resource namespacing and structure".

@Oberon00 Oberon00 changed the title Consistent attribute definitions Consistent allowed attribute types Feb 6, 2020
@arminru
Copy link
Member

arminru commented Feb 6, 2020

related: @jmacd's issue #387 about restricting metric and label names

@bogdandrutu
Copy link
Member

@Oberon00 I am not sure exactly what is required to "fix" this issue, can you summarize?

@Oberon00
Copy link
Member Author

Oberon00 commented Feb 13, 2020

I think we should split attributes in their own api-attributes document and reference them in all the other places (probably only allowing primitive types for metric label values). Coincidentally, this is also how opentelemetry-proto is currently implemented.

@jmacd
Copy link
Contributor

jmacd commented Mar 3, 2020

My opinion is that all of the attributes in OpenTelemetry should have the same type and structure. I can't see any reasons why any key:value might not appear as any one of the above five kinds of attribute (span attribute, span link attribute, span event attribute, resource, metric) or as a Correlation Context value. Semantically, I believe there is no difference between using a resource as compared with a span attribute. Likewise, it means the same thing if an attribute appears in a metric label set or as a resource.

For example, a hostname attribute is commonly applied as a resource, but a proxy that forwards data from multiple hosts might attach it as a span attribute or a metric label.

There is a concern, I'm aware, that some attribute value types are not commonly accepted in some systems. For example, metric systems typically accept label values in string form. All values have a canonical translation to a string format, and I believe we should specify that this as the standard behavior for coercing attributes into export formats. If an export format does not support null-valued or number-valued labels, then those values should be converted to a string for export. Likewise, if an export format does not support multi-valued list attributes, those values should be coerced to comma-separated values in the exporter.

@bogdandrutu
Copy link
Member

So we have 1-4 all using and referring to the properties as attributes. I think Metrics calls the properties "labels" and not attributes so it is clear that it is a distinction, more than that all systems that I know of use string only values). Also Correlation context as defined in w3c supports only string values.

So we still have two groups: one that supports all attributes values and one that only supports string.

@Oberon00
Copy link
Member Author

"Also Correlation context as defined in w3c supports only string values"
We could still add encoding on top of the correlation context I suppose. But I agree that's an argument.

@bogdandrutu bogdandrutu added area:api Cross language API specification issue spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory labels Jun 26, 2020
@bogdandrutu
Copy link
Member

Closing this since I think we resolved the issue: resource, events, links, span use attributes; metrics use labels;

@Oberon00
Copy link
Member Author

That may be how the spec is understood and implemented today, but not what it says. Resources still have their own definition for allowed values and do not reference the definition of Attributes that is in the span API (where it probably shouldn't be if it is to be shared). See https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/sdk.md#create

@Oberon00 Oberon00 reopened this Jun 26, 2020
@Oberon00
Copy link
Member Author

Maybe this just needs a small cleanup PR and not much discussion, but it is still unsolved in the current specification text.

@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 2, 2020
@andrewhsu andrewhsu added the priority:p1 Highest priority level label Jul 17, 2020
@jmacd
Copy link
Contributor

jmacd commented Jul 17, 2020

It would be great if we could stop thinking labels and attributes as different things. When a span is used to construct a metric, we're going to convert attributes into labels and -- they're the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory
Projects
None yet
7 participants