-
Notifications
You must be signed in to change notification settings - Fork 650
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 LogEmitter.flush() to align with OTel Log Spec #2863
Remove LogEmitter.flush() to align with OTel Log Spec #2863
Conversation
@srikanthccv Would you mind running the workflow checks again so I can confirm there are no more changes that need to be made from my side? |
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.
Overall LGTM, Please rebase and also update the example here https://github.com/open-telemetry/opentelemetry-python/blob/main/docs/examples/logs/example.py.
64b9dea
to
73bc04d
Compare
@srikanthccv Done! Update: For clarity, added to summary of changes in original post |
Update:
|
Unfortunately, pulling to my local feature branch (after rebasing the remote feature branch here on GitHub) caused a merge commit :( |
All your commits will be squashed before merging. |
4591d67
to
df7b810
Compare
…itter_provider, Obtain log_emitter via log_emitter_provider
…tterProvider.force_flush()
Error in atexit._run_exitfuncs: Traceback (most recent call last): File "C:\Program Files\Python39\lib\logging\__init__.py", line 2129, in shutdown h.flush() File "C:\...\.tox\opentelemetry-sdk\lib\site-packages\opentelemetry\sdk\_logs\__init__.py", line 373, in flush self._log_emitter_provider.force_flush() AttributeError: 'DummyLogEmitterProvider' object has no attribute 'force_flush'
…) to match LogEmitterProvider.get_log_emitter()
df7b810
to
6373a56
Compare
Thanks, that's good to know! Having the merge+duplicate commits hanging around was still bugging me though, so I ended up digging into rebase until I was able to fix it! 🎉 |
Glad to see it merged -- thanks for the guidance, @srikanthccv! |
Feel free to pick some new issues. I will be happy to help you. |
Description
Fixes #2584
Motivation: Align
LogEmitter
implementation with OTel Log specSummary:
LogEmitter.flush()
LoggingHandler.__init__()
:log_emitter
arg withlog_emitter_provider
log_emitter
vialog_emitter_provider
LoggingHandler.flush()
: ReplaceLogEmitter.flush()
withLogEmitterProvider.force_flush()
LoggingHandler.__init__()
signatureDummyLogEmitterProvider
DummyLogEmitter.flush()
to align with removal ofLogEmitter.flush()
LoggingHandler.__init__()
signatureps: First time contributing -- let me know if you need me to change/add anything!
Type of change
How Has This Been Tested?
opentelemetry-sdk
test suiteTest Results
Test Results
Test Results
Test Results
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/
oropentelemetry-sdk/
The method interfaces of
test/util
have changedScripts in
scripts/
that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.toml
isort.cfg
.flake8
When a new
.github/CODEOWNER
is addedMajor changes to project information, such as in:
README.md
CONTRIBUTING.md
Yes. (I think so, based on the first example)
Checklist: