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

Support maps and heterogeneous arrays as attribute values #2888

Closed
wants to merge 10 commits into from
+32 −11
Closed

Support maps and heterogeneous arrays as attribute values #2888

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8c497e3
Support maps and heterogeneous arrays as attribute values
tigrannajaryan Oct 17, 2022
c0ef00f
Changes to address PR comments
tigrannajaryan Jan 10, 2024
c2d2941
Add a note that reference loops are not allowed
tigrannajaryan Jan 10, 2024
1a385e7
Completely prohibit complex types for metric attributes
tigrannajaryan Jan 11, 2024
b15695c
Fixed a couple minor incorrect/missing bits
tigrannajaryan Jan 11, 2024
5e0d081
Use "cycle" instead of "loops"
tigrannajaryan Jan 11, 2024
5ccaaf0
Fix wording compliance matrix
tigrannajaryan Jan 11, 2024
dc31a43
Add line for traces in compliance matrix
tigrannajaryan Jan 11, 2024
4f5e141
Remove line item for metric in compliance matrix since it is not need…
tigrannajaryan Jan 11, 2024
58cbb1a
Add line item for logs in compliance matrix
tigrannajaryan Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ release.
- Clarify that attribute keys are case-sensitive.
([#3784](https://github.com/open-telemetry/opentelemetry-specification/pull/3784))

- Support maps and heterogeneous arrays as attribute values
([#2888](https://github.com/open-telemetry/opentelemetry-specification/pull/2888))

### Supplementary Guidelines

## v1.28.0 (2023-12-07)
Expand Down
2 changes: 2 additions & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ formats is required. Implementing more than one format is optional.
| A metric Producer accepts an optional metric Filter | | | | | | | | | | | | |
| The metric Reader implementation supports registering metric Filter and passing them its registered metric Producers | | | | | | | | | | | | |
| The metric SDK's metric Producer implementations uses the metric Filter | | | | | | | | | | | | |
| Non-homogeneous arrays and maps (including nested) | | | | | | | | | | | | |
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved

## Logs

Expand Down Expand Up @@ -242,6 +243,7 @@ Disclaimer: this list of features is still a work in progress, please refer to t
| Feature | Optional | Go | Java | JS | Python | Ruby | Erlang | PHP | Rust | C++ | .NET | Swift |
|---------------------------------------------------------------------------------------------------------------------------------------------|----------|----|------|----|--------|------|--------|-----|------|-----|------|-------|
| Create from Attributes | | + | + | + | + | + | + | + | + | + | + | + |
| Non-homogeneous arrays and maps (including nested) for attribute values | | | | | | | | | | | | |
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
| Create empty | | + | + | + | + | + | + | + | + | + | + | + |
| [Merge (v2)](specification/resource/sdk.md#merge) | | + | + | | + | + | + | + | + | + | + | |
| Retrieve attributes | | + | + | + | + | + | + | + | + | + | + | + |
Expand Down
19 changes: 15 additions & 4 deletions specification/common/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,23 @@ An `Attribute` is a key-value pair, which MUST have the following properties:

- The attribute key MUST be a non-`null` and non-empty string.
- Case sensitivity of keys is preserved. Keys that differ in casing are treated as distinct keys.
- The attribute value is either:
- The attribute value can be of `any` type, where any is defined as one of the following:
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
- A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed 64 bit integer.
- An array of primitive type values. The array MUST be homogeneous,
i.e., it MUST NOT contain values of different types.
- A homogeneous array of values of primitive type [before 1.29.0].
- An array of `any` values [since 1.29.0].
- A key/value map, where key is string and value is `any` value. Any form of reference
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
loops is disallowed. [since 1.29.0].
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@pellared pellared Jan 11, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if will be possible to disallow it in all languages (e.g. Python).

Maybe we should say something

Suggested change
loops is disallowed. [since 1.29.0].
cycle is MUST NOT be supported. [since 1.29.0].

E.g. in .NET the attribute is passed as KeyValuePair<string, object>. Nothing prevents from passing an object which will have a cycle, but the SDK can handle it e.g. by ignoring such attribute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even when it is possible I am not sure if it would be efficient to check if there is no cycle during "attribute value construction". I think it would be better if the place where attributes are accepted would check if the attributes' values have no cycles.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we need to specify where exactly (which place in code) this check happens. We only describe a valid data model here. How you enforce that data model is a particular SDK's design choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only describe a valid data model here. How you enforce that data model is a particular SDK's design choice.

I totally agree but the current way it is worded suggests that it should be impossible to pass ("is disallowed") such attributes. This is why a propose to change the wording to "supported". This section does not says anything about the SDK so it can be interpreted that even the API disallows. Also I do not know how to interpret what "disallowed" means from programming perspective.


For protocols that do not natively support non-string values, non-string values SHOULD be represented as JSON-encoded strings. For example, the expression `int64(100)` will be encoded as `100`, `float64(1.5)` will be encoded as `1.5`, and an empty array of any type will be encoded as `[]`.
Complex attribute types (such as arrays and maps):

- SHOULD be used sparingly, in situations where their use minimizes manipulation of
the data’s original structure.
- SHOULD NOT be used with metrics.
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these are only guidelines for the users (consumers)? If so then I think it will be good to call it out explicitly that these are usage guidelines. Also I do not think it we should use normative wording as we "do not control the user".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for producers and designers of semantic conventions. I am not sure how this can be interpreted to be guidelines for consumers. Consumers don't choose what attribute types to use, they consume what they receive.

Also I do not think it we should use normative wording as we "do not control the user".

We do control Otel semantic conventions and I think we are in the position to make a strong recommendation to producers, even the ones we don't control.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarification: By consumers I had in mind "users of the API" 😉 Feel free to resolve the comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is guidelines for semantic conventions, we should move it to the semantic conventions repo. We recently moved attribute naming docs from the spec to semantic conventions. It doesn't make sense to have semantic convention advice interspersed in a document describing attributes for SDK implementers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-berg The first list item is a guideline for semantic conventions and for anyone who uses the API. Perhaps semantic conventions repo is a better place for it.

I have changed the second list item to MUST NOT, which makes it an API requirement, not a guideline, so it belongs here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarification: By consumers I had in mind "users of the API" 😉 Feel free to resolve the comment.

Ah, I misunderstood what you meant :-) Good question in that case.

I think we can do as @jack-berg suggests and move the usage recommendation to semconv and keep the strict API requirement here.


When exporting to protocols that do not natively support a particular non-string
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
value type the value should be converted to a string JSON-encoding of the value.
For example, the expression `int64(100)` will be encoded as `100`, `float64(1.5)` will
be encoded as `1.5`, and an empty array of any type will be encoded as `[]`.
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved

Attribute values expressing a numerical value of zero, an empty string, or an
empty array are considered meaningful and MUST be stored and passed on to
Expand Down
4 changes: 2 additions & 2 deletions specification/trace/sdk_exporters/jaeger.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ OpenTelemetry Span `Attribute`(s) MUST be reported as `tags` to Jaeger.

Primitive types MUST be represented by the corresponding types of Jaeger tags.

Array values MUST be serialized to string like a JSON list as mentioned in
[semantic conventions](../../overview.md#semantic-conventions).
Array and map values MUST be serialized to a JSON and recorded as a tag of string type
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
as mentioned in [attribute value definition](../../common/README.md#attribute).

### Links

Expand Down
4 changes: 2 additions & 2 deletions specification/trace/sdk_exporters/zipkin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@ document maps to the strongly-typed fields of Zipkin spans.
Primitive types MUST be converted to string using en-US culture settings.
Boolean values MUST use lower case strings `"true"` and `"false"`.

Array values MUST be serialized to string like a JSON list as mentioned in
[semantic conventions](../../overview.md#semantic-conventions).
Array and map values MUST be serialized to a JSON and recorded as a tag of string type
as mentioned in [attribute value definition](../../common/README.md#attribute).

TBD: add examples

Expand Down