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

Clarification on what an Event is, and what the event.domain and event.name attributes represent #2848

Merged
merged 12 commits into from
Nov 9, 2022

Conversation

scheler
Copy link
Contributor

@scheler scheler commented Sep 30, 2022

Changes

This PR provides clarification on what an Event is, and what the event.domain and event.name attributes represent.

This PR also removes the text around event.name being unique in a domain, as it is wrongly worded and is not required. This came up in open-telemetry/opentelemetry-collector-contrib#14474. The attribute event.domain is meant to provide a logical separation across different event systems, so it allows for similar looking names in different domains but with a completely different purpose. However, within each domain we don't have to have any uniqueness constraints on event.name and leave it to whatever is idiomatic to the particular event system the domain represents.

For non-trivial changes, follow the change proposal
process
and link to the related issue(s)
and/or OTEP(s), update the
CHANGELOG.md, and also be sure to update
spec-compliance-matrix.md if necessary.

Related issues #

Related OTEP(s) #

This came up in
open-telemetry/opentelemetry-collector-contrib#14474
where it was highlighted that just `name` is not sufficient to provide
for uniqueness of a Kubernetes Event.

The attribute `event.domain` is only meant to be a logical separation
across different event systems, so it allows for similar looking names
in different domains but with a completely different purpose. However,
within each domain we don't have to have any uniqueness constraints on
the `name` and leave it to whatever is idiomatic to the particular event
system the domain represents.
@scheler scheler changed the title Remove language around uniqueness of event names within a domain Further clarification on event name and domain Oct 9, 2022
@scheler scheler changed the title Further clarification on event name and domain Clarification on what an Event is, and what the event.domain and event.name attributes represent Oct 25, 2022
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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 2, 2022
@tigrannajaryan
Copy link
Member

@open-telemetry/specs-logs-approvers please review.

@tigrannajaryan
Copy link
Member

@scheler please fix the build.

@scheler
Copy link
Contributor Author

scheler commented Nov 5, 2022

@tigrannajaryan the build is fixed now. Regarding the 2 attributes being on the LogAttributes in the Event definition, it's already mentioned that way in the spec, see here. Given that we are moving in this direction with the proposal for Events API layered on top of Logger API #2917, maybe it's ok to keep this language as is.

@tigrannajaryan
Copy link
Member

Regarding the 2 attributes being on the LogAttributes in the Event definition, it's already mentioned that way in the spec, see here. Given that we are moving in this direction with the proposal for Events API layered on top of Logger API #2917, maybe it's ok to keep this language as is.

I would still prefer to remove it from this PR until we agree that event.domain moves from the Scope to LogRecord and update the spec everywhere. We don't have such an agreement.

@scheler
Copy link
Contributor Author

scheler commented Nov 9, 2022

Regarding the 2 attributes being on the LogAttributes in the Event definition, it's already mentioned that way in the spec, see here. Given that we are moving in this direction with the proposal for Events API layered on top of Logger API #2917, maybe it's ok to keep this language as is.

I would still prefer to remove it from this PR until we agree that event.domain moves from the Scope to LogRecord and update the spec everywhere. We don't have such an agreement.

@tigrannajaryan made the change. Updated with the description from https://github.com/open-telemetry/opentelemetry-specification/pull/2863/files

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan
Copy link
Member

@open-telemetry/specs-logs-approvers please review.

@tigrannajaryan
Copy link
Member

As agreed in the Log SIG we are merging this as is. A separate PR will be created to move event.domain to the LogRecord. @scheler will you do that?

@tigrannajaryan tigrannajaryan merged commit b1d14b5 into open-telemetry:main Nov 9, 2022
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

7 participants