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

Define null as an invalid value for attributes and declare attempts to set null as undefined behavior #992

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Define null as an invalid value for attributes and declare attempts to set null as undefined behavior #992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa9aa51
Define `null` as an invalid value for attributes and declare attempts…
arminru Sep 23, 2020
bdbef78
Remove explicit instruction to avoid calling SetAttribute with null
arminru Sep 24, 2020
39c6260
Add spec compliance matrix entry
arminru Sep 24, 2020
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 @@ -70,6 +70,9 @@ Updates:
([#914](https://github.com/open-telemetry/opentelemetry-specification/pull/914))
- Remove obsolete `http.status_text` from semantic conventions
([#972](https://github.com/open-telemetry/opentelemetry-specification/pull/972))
- Define `null` as an invalid value for attributes and declare attempts to set
`null` as undefined behavior
([#992](https://github.com/open-telemetry/opentelemetry-specification/pull/992))
- SDK: Rename the `Decision` values for `SamplingResult`s to `DROP`, `RECORD_ONLY`
and `RECORD_AND_SAMPLE` for consistency
([#938](https://github.com/open-telemetry/opentelemetry-specification/pull/938),
Expand Down
1 change: 1 addition & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ status of the feature is not known.
|Double floating-point type | + | + | + | + | + | + | - | + | + | + |
|Signed int64 type | + | + | + | + | + | + | - | + | + | + |
|Array of primitives (homogeneous) | + | + | + | + | + | - | + | + | + | + |
|`null` values documented as invalid/undefined | | | | | | | | | | |
|Unicode support for keys and string values | + | + | + | + | + | + | + | + | + | + |
|[Span linking](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#add-links)|
|AddLink | + | + | + | + | + | + | + | + | - | + |
Expand Down
8 changes: 4 additions & 4 deletions specification/common/common.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ Attributes SHOULD preserve the order in which they're set.

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. Attribute values of `null` are considered to be not set
and get discarded as if that `Attribute` has never been created.
As an exception to this, if overwriting of values is supported, this results in
removing the attribute.
processors / exporters.

Attribute values of `null` are not valid and attempting to set a `null` value is
arminru marked this conversation as resolved.
Show resolved Hide resolved
undefined behavior.
Copy link
Member

Choose a reason for hiding this comment

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

I recommend adding a sentence that recommends SIGs to use @Nonnull or other compile-time / lint-time indicators to prevent null if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please let's do this in a follow up, as it's a) sugar on top and b) This change has been delayed forever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me.

@carlosalberto @tigrannajaryan @jmacd since you were opposed to the previous instruction targeted to users, would you be fine with something like that?

Suggested change
undefined behavior.
undefined behavior.
APIs SHOULD be annotated accordingly, if compile-time/lint-time hints are available
in the respective language (e.g., `@Nonnull` in Java).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I missed that comment @carlosalberto. I'm fine with merging without this but maybe we happen to find a quick agreement on that detail and can do it at once.


`null` values within arrays MUST be preserved as-is (i.e., passed on to span
processors / exporters as `null`). If exporters do not support exporting `null`
Expand Down