-
Notifications
You must be signed in to change notification settings - Fork 647
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
Events API implementation #4054
Conversation
@lzchen Could you please have a look at this? |
I think there are a few things missing from the api, such as setting the global provider, the corrensponding |
Please add a CHANGELOG.md entry. |
Changed the description. Will create a separate PR once this is merged. |
Thanks for the contribution. Please add some tests to test the api is working correctly. |
@lzchen All the classes in this code change are abstract and I was wondering if you could tell me how I could test it? I was looking at the tests for logs and only the NoOpLogger and ProxyLogger are tested as they inherit the abstract classes. |
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.
Maybe rename this PR and the directory to "Event" not "Events"?
Maybe we should double check with the documentation people. Need to update the heading here too 😅 https://opentelemetry.io/docs/specs/otel/logs/event-api/. Also the "Events SDK" is plural. |
hah yeah after I made that comment I realized there is no consistency quorum across langs on Events vs Event 🤷, nagging my favorite linter @svrnm |
Are you still working on this? If you can address the remaining points we can get this PR merged quickly :) |
Sorry, have been a bit busy. Will have it fixed by the weekend. |
@jack-berg do you mind doing a pass despite this being in python? I know practice is different per lang, but possibly there are some corner cases, tests or otherwise that pop out at you as being wise to handle. |
We have a couple of folks who want to test the functionality of the events api so this PR would be blocking for them. Would you be able to streamline the work for this or would it be okay for someone else to take over? |
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.
To move things forward we can merge this and I can followup with the fix for the default attributes. Or a maintainer can fix that if you prefer.
Co-authored-by: Riccardo Magliocchetti <[email protected]>
Trying to figure out how to make use of this! Any hints or examples would be appreciated. If I figure it out in the interim, I'll edit this comment accordingly. |
@bradbeattie take a look at sdk tests #4176 |
Description
Addresses #3053
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Checklist: