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

Define Event API arguments and mapping to log record #3772

Merged
merged 8 commits into from Dec 1, 2023

Conversation

jack-berg
Copy link
Member

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 should be exposed in the Event API
  • LogRecord#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 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 Merge event's domain and name #3749) should be an AnyValue 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 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.

Copy link

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

Apart from my comment about the event.domain rewording and including portions of #3749

LGTM.

@tigrannajaryan
Copy link
Member

  • Therefore, I've indicated that Event API implementations should set a default SeverityNumber of 10.

Why 10? Are we trying to be in the middle of the "info" range for some reason? Note that mappings of other formats to Otel use INFO=9 as the level for informational events. Should we use the same level for Otel-generated events?

I agree with all other assertions in the PR description.

specification/logs/event-api.md Outdated Show resolved Hide resolved
specification/logs/event-api.md Outdated Show resolved Hide resolved
@jack-berg
Copy link
Member Author

Why 10? Are we trying to be in the middle of the "info" range for some reason? Note that mappings of other formats to Otel use INFO=9 as the level for informational events. Should we use the same level for Otel-generated events?

We discussed that a good default severity would be an "info" level, but I took some agency with the selection of 10 to stay in the middle of the info range. I don't feel strongly about this though and the appendix you point to seems to be a pretty good reason to select INFO=9 as the default.

@jack-berg
Copy link
Member Author

Draft PR where I've prototyped these changes in opentelemetry-java: open-telemetry/opentelemetry-java#6001

@tigrannajaryan
Copy link
Member

Event domain and name are now merged, I think this needs to be updated to reflect that (or a separate is needed to do just that).

@jack-berg
Copy link
Member Author

I've rebased this now that event domain and name are merged in #3749.

@carlosalberto
Copy link
Contributor

@jack-berg CHANGELOG entry?

@jack-berg
Copy link
Member Author

Changelog entry added @carlosalberto 👍

@jack-berg
Copy link
Member Author

Will merge tomorrow if there are no additional comments.

@jack-berg jack-berg merged commit fecfb7d into open-telemetry:main Dec 1, 2023
7 checks passed
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants