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

Merge event's domain and name #3749

Merged

Conversation

patrickhousley
Copy link
Contributor

@patrickhousley patrickhousley commented Oct 30, 2023

Fixes #2994

Changes

Merging the domain and name fields for events and modifying language to refer to the first part of the name as namespace

@patrickhousley patrickhousley requested review from a team October 30, 2023 14:31
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@patrickhousley patrickhousley changed the title Merging domain and name Merging events domain and name Oct 30, 2023
Copy link

github-actions bot commented Nov 7, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 7, 2023
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Note that this change should be made in tandem with open-telemetry/semantic-conventions#473.

specification/logs/event-api.md Show resolved Hide resolved
specification/logs/event-api.md Outdated Show resolved Hide resolved
@jsuereth jsuereth added spec:logs Related to the specification/logs directory triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal labels Nov 15, 2023
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Can you add a changelog entry?

Looks good to me otherwise.

specification/logs/event-api.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

Now that #3749 has been merged, is this ready to go?

CHANGELOG.md Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan changed the title Merging events domain and name Merge event's domain and name Nov 27, 2023
@carlosalberto carlosalberto merged commit 13860cb into open-telemetry:main Nov 28, 2023
7 checks passed
jack-berg added a commit that referenced this pull request Dec 1, 2023
In the 11/17/23 Event SIG we discussed and reached agreement on a number
of things related to the Event API and how its parameters should
manifest on resulting log record:

- Some, but not all the parameters of [emit a
logRecord](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#emit-a-logrecord)
should be exposed in the Event API
-
[LogRecord#SeverityText](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-severitytext)
should not be exposed as an Event API parameter. The SeverityText is
used to record the original severity as recorded in the original log
framework. The Event API is not for bridging log frameworks and this
this field does not make sense to expose. The Event API implementation
should leave SeverityText blank.
-
[LogRecord#ObservedTimestamp](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-observedtimestamp)
should not be as exposed as an Event API parameter. The semantics of
ObservedTimestamp are defined as "Time when the event was observed by
the collection system". For the Event API, the ObserveTimestamp is
always when the Event is emitted.
- The Event Payload, which conforms to the well defined schema for a
particular `event.name` (event.domain is going away in #3749) should be
an
[AnyValue](https://github.com/open-telemetry/opentelemetry-proto/blob/ea449ae0e9b282f96ec12a09e796dbb3d390ed4f/opentelemetry/proto/common/v1/common.proto#L28)
and recorded to the log record body.
- We need some way to sample / filter Events based on the notion of
priority / severity. We decided to use the
[LogRecord#SeverityNumber](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-severitynumber)
field for this purpose rather than introducing a new concept.
LogRecord#SeverityNumber is optional, and we want the equivalent Event
API parameter to be optional as well. Therefore, I've indicated that
Event API implementations should set a default SeverityNumber of 10.
Without choosing a default, future sampling strategies that are based on
SeverityNumber will filter away all Events which do not explicitly set a
SeverityNumber, which is not desirable.

Note that there is going to be a corresponding PR in
`semantic-conventions` which says that Events put their payload in the
LogRecord#Body field.

---------

Co-authored-by: Carlos Alberto Cortez <calberto.cortez@gmail.com>
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
jack-berg pushed a commit that referenced this pull request Dec 1, 2023
"merge conflict" between #3749 and #3772.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide if event.domain separately from event.name is necessary
6 participants