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

Merging events domain and name #473

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. This more closely aligns the semantic conventions of events with metrics as we have discussed during the events working group meeting.

The spec update pull request is here.

Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.

Merge requirement checklist

@patrickhousley patrickhousley requested review from a team as code owners October 30, 2023 14:51
CHANGELOG.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Looks good to me, and matches what we talked about in the sig meeting. Do the protobuf definitions also need to change accordingly? Can we link to that work or create a follow-up issue? Thanks! 🙏🏻

@patrickhousley
Copy link
Contributor Author

Looks good to me, and matches what we talked about in the sig meeting. Do the protobuf definitions also need to change accordingly? Can we link to that work or create a follow-up issue? Thanks! 🙏🏻

I don't see events in the protobuf definitions repo. I may be blind so could you drop me a link? If it's not there, we should get a ticket created to make the protobuf definition.

@tigrannajaryan
Copy link
Member

The PR needs a description that explains why the change is made, the motivation and reasoning.

It links to issue #2994 which does not exist in this repo.

docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
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.

I am temporarily blocking the PR to give time to @scheler as they requested: open-telemetry/opentelemetry-specification#2994 (comment)

@scheler please comment on how much time you think you need.

The work on this PR can continue, it is a temporary block to avoid premature merging.

Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@patrickhousley patrickhousley changed the title Merging domain and name Merging events domain and name Nov 7, 2023
docs/general/events.md Outdated Show resolved Hide resolved
docs/general/events.md Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan dismissed their stale review November 10, 2023 18:04

Removing my block after confirming with @scheler

@patrickhousley
Copy link
Contributor Author

We're at 5 approvals now, so I think we're getting close. Looks like there's a table issue right now @patrickhousley

File /spec/general/events.md contains a table that would be reformatted.
make: *** [Makefile:98: table-check] Error 1

I run make fix locally but it's like it's reverting changes to the files. I am not sure what the error is or what the fix is doing.

docs/mobile/events.md Outdated Show resolved Hide resolved
@patrickhousley
Copy link
Contributor Author

@breedx-splk I believe we are good to go now.

docs/general/events.md Show resolved Hide resolved
model/logs/events.yaml Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan dismissed their stale review November 22, 2023 15:43

My request to link to main branch was wrong

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Please change the links to the spec repo to use the correct tag.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Looks good, but the PR does more things then it says in the description. It also adds the device prefix, which I couldn't find any decision or discussion about it in the comments (maybe I missed it?)

I saw this issue #548 which opens the discussion but also no conclusion. Since this has several approvals, is everyone OK with this change as well? Is everyone aware of this?

CHANGELOG.md Outdated Show resolved Hide resolved
@AlexanderWert
Copy link
Member

AlexanderWert commented Nov 23, 2023

It also adds the device prefix, which I couldn't find any decision or discussion about it in the comments (maybe I missed it?)

@joaopgrassi Actually it does not add the device prefix, but for mobile merges the event.domain(which has the value device) into the event.name as a prefix. So, actually applies the general proposal to the only relevant, concrete use case so far.

I'd like to keep #548 separate from this PR, that would change the prefix from device to mobile in that case.

@AlexanderWert AlexanderWert merged commit 12dfe6a into open-telemetry:main Nov 24, 2023
9 checks passed
@patrickhousley patrickhousley deleted the event_space_namespace_update branch November 27, 2023 13:17
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
Co-authored-by: Joao Grassi <joao.grassi@dynatrace.com>
Co-authored-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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