-
Notifications
You must be signed in to change notification settings - Fork 831
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
Remove domain from event api. #6253
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6253 +/- ##
============================================
- Coverage 91.06% 91.05% -0.01%
Complexity 5695 5695
============================================
Files 621 621
Lines 16667 16658 -9
Branches 1707 1707
============================================
- Hits 15177 15168 -9
+ Misses 997 996 -1
- Partials 493 494 +1 ☔ View full report in Codecov by Sentry. |
api/events/src/test/java/io/opentelemetry/api/events/DefaultEventEmitterTest.java
Outdated
Show resolved
Hide resolved
…entEmitterTest.java Co-authored-by: Trask Stalnaker <[email protected]>
In #6001 I made this change, along with other changes reflecting spec changes like adding a payload field, setters for severity, context, etc. There are still a couple of open questions whose answers would churn the API / user experience:
I think making all of these changes in one release would be optimal from the users standpoint, since they only need to digest the breaking changes once. But there is also an advantage to staying relatively in line with the spec, even if all the questions haven't been hashed out. WDYT? cc @open-telemetry/java-approvers |
👍 but also ok with me to merge if someone wants this particular change sooner |
Looking for input from other approvers as well. I definitely wanted to align the API with the spec, since it was one of the first decisions the event WG made. My thinking is that users would hopefully favor changes in unstable APIs over having APIs that don't match the spec. |
we've gotten feedback from @brunobat and @jonatan-ivanov that breaking changes in unstable APIs has created a lot of extra work for them, but I'm not sure if there's much use of the event api yet, so maybe no need to consolidate breaking changes into a single release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using it yet, but we plan to. It should be fine.
Thanks for the heads-up, it should be ok from Micrometer perspective too. |
Now that the |
What do you think about targeting the April release to group these changes together? |
Works for me. 👍🏻 |
See open-telemetry/semantic-conventions#473 for details. The events working group has combined the
event.name
andevent.domain
by just having dot-separated prefixes on event names. This updates the API to match that expectation.