Skip to content

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
@codecov
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

6 participants