-
Notifications
You must be signed in to change notification settings - Fork 733
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
add Events SDK #4629
add Events SDK #4629
Conversation
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.
Thanks for working on this! I added a few non-blocking suggestions to make it easier for someone to try it out. I also added a note about setting a default severityNumber of 9 based on the spec.
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.
A few comments, looks good overall 👍
This adds details to how EventLoggerProvider and EventLogger should be implemented in the default Events SDK. Prototypes - [JavaScript prototype](open-telemetry/opentelemetry-js#4629) - [Java prototype](https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/internal/SdkEventLoggerProvider.java)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4629 +/- ##
==========================================
+ Coverage 91.04% 93.82% +2.78%
==========================================
Files 89 240 +151
Lines 1954 5606 +3652
Branches 416 1125 +709
==========================================
+ Hits 1779 5260 +3481
- Misses 175 346 +171 |
Which problem is this PR solving?
This adds an experimental implementation of the Events SDK.
As a related update, I have also updated the Events API with the following:
Type of change
Checklist: