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

Remove domain from event api. #6253

Merged
merged 5 commits into from
Mar 12, 2024
Merged

Conversation

breedx-splk
Copy link
Contributor

See open-telemetry/semantic-conventions#473 for details. The events working group has combined the event.name and event.domain by just having dot-separated prefixes on event names. This updates the API to match that expectation.

@breedx-splk breedx-splk requested a review from a team February 27, 2024 00:56
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.05%. Comparing base (1d4a2f4) to head (f57a52a).
Report is 1 commits behind head on main.

Files Patch % Lines
...try/sdk/logs/internal/SdkEventEmitterProvider.java 80.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

…entEmitterTest.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@jack-berg
Copy link
Member

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

@trask
Copy link
Member

trask commented Feb 29, 2024

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 also ok with me to merge if someone wants this particular change sooner

@breedx-splk
Copy link
Contributor Author

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.

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.

@trask
Copy link
Member

trask commented Feb 29, 2024

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

Copy link
Contributor

@brunobat brunobat left a 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.

@jonatan-ivanov
Copy link
Member

Thanks for the heads-up, it should be ok from Micrometer perspective too.

@breedx-splk
Copy link
Contributor Author

Now that the EventLogger name is decided, there are going to be a series of changes around events. Would be nice to get this going as a start...

@jack-berg
Copy link
Member

What do you think about targeting the April release to group these changes together?

@breedx-splk
Copy link
Contributor Author

What do you think about targeting the April release to group these changes together?

Works for me. 👍🏻

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.

6 participants