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

Support maps and heterogeneous arrays as attribute values #2888

Closed
wants to merge 10 commits into from
Closed

Support maps and heterogeneous arrays as attribute values #2888

Show file tree
Hide file tree
Changes from all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
3 changes: 3 additions & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ formats is required. Implementing more than one format is optional.
| Array of primitives (homogeneous) | | + | + | + | + | + | + | + | + | + | + | + |
| `null` values documented as invalid/undefined | | + | + | + | + | + | N/A | + | | + | | N/A |
| Unicode support for keys and string values | | + | + | + | + | + | + | + | + | + | + | + |
| Non-homogeneous arrays and maps (including nested) for attribute values | | | | | | | | | | | | |
| [Span linking](specification/trace/api.md#specifying-links) | Optional | Go | Java | JS | Python | Ruby | Erlang | PHP | Rust | C++ | .NET | Swift |
| Links can be recorded on span creation | | + | + | | + | + | + | + | + | + | + | |
| Links can be recorded after span creation | | | | | | | | | | + | | |
Expand Down Expand Up @@ -236,12 +237,14 @@ Disclaimer: this list of features is still a work in progress, please refer to t
| OTLP File exporter | | | - | | - | | | | | | - | |
| Can plug custom LogRecordExporter | | | + | | | | | + | | + | | |
| Trace Context Injection | | | + | | + | | | + | | + | + | |
| Non-homogeneous arrays and maps (including nested) for attribute values| | | | | | | | | | | | |
Copy link
Contributor

@MSNev MSNev Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

JavaScript has this as experimental as LogAttributes
https://github.com/open-telemetry/opentelemetry-js/blob/bf8714edcba8a2384db857696a1608215548d182/experimental/packages/api-logs/src/types/LogRecord.ts#L20-L22


## Resource

| 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
29 changes: 22 additions & 7 deletions specification/common/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,31 @@ 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.
- An array of values of primitive type [before 1.29.0].
Copy link
Member

Choose a reason for hiding this comment

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

Should we also include a byte array (as for logs attributes)?

- 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
cycle is disallowed. [since 1.29.0].

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. They may
Copy link
Contributor

Choose a reason for hiding this comment

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

This normative statement is meaningless. Users define attributes not OTel SDKs.

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 semantic convention authors. Will be moved to semconv repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is for semantic convention authors. Will be moved to semconv repo.

Makes sense. Can we track that with an issue if this merges?

be useful for example in situations where their use minimizes manipulation
of the data’s original structure.

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
processors / exporters.
The following value types MUST NOT be used as metric attribute values:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not viable. Stable implementations have already been released for this signal. Those implementations already accept attributes. Adding this statement here is hiding a breaking change.

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 actually implemented this and found it to be possible in non-breaking manner for Java.

Do you think it is impossible to do for Go in a non-breaking way?

Copy link
Contributor

@MrAlias MrAlias Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the link to the example implementation, I missed that. I'm not that knowledgeable in Java, but I don't see in that implementation where it restricts attributes values in the metric signal.

The Go implementation passes the same type attribute.KeyValue to both the metric and trace signal. We will not be able to allow some values to be passed to the trace signal but not the metric signal.

Copy link
Member Author

@tigrannajaryan tigrannajaryan Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, that is very important. If we think this is impossible to implement in non-breaking way then it is a blocker, we can't accept the PR.

I would like to take a closer look at Go implementation and see if we can find a non-breaking way to apply this change.

Since this came up I would also want to be more careful and check some other languages (strongly typed/compiled languages are most likely to be impacted).

Copy link
Member

@pellared pellared Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the alternative could be to describe in metrics SDK specification how the SDK should/must handle such cases (when these types of attributes are passed).

Moreover, it would be good to add a rationale what are the reasons to have these constraints.


- Non-homogeneous arrays (arrays containing values of different types).
- Nested arrays.
- Key/value maps.

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, an
empty array or empty key/value map are considered meaningful and MUST be stored and
passed on to processors / exporters.

Attribute values of `null` are not valid and attempting to set a `null` value is
undefined behavior.
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
Loading