-
Notifications
You must be signed in to change notification settings - Fork 821
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
fix(sdk-logs): hide internal methods with internal shared state #3865
fix(sdk-logs): hide internal methods with internal shared state #3865
Conversation
adee4ee
to
650c6e7
Compare
Codecov Report
@@ Coverage Diff @@
## main #3865 +/- ##
==========================================
+ Coverage 92.30% 92.60% +0.29%
==========================================
Files 328 295 -33
Lines 9385 8249 -1136
Branches 1993 1712 -281
==========================================
- Hits 8663 7639 -1024
+ Misses 722 610 -112 |
650c6e7
to
b46fef9
Compare
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
@@ -15,7 +15,6 @@ | |||
*/ | |||
|
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.
We may want to remove LogRecord
and Logger
from the exports in this file. Logger
should only be be aquired though LoggerProvider
, and the only api.LogRecord
should be passed to emit
IIUC, so no need to let users create instances (now impossible anyway as we don't export LoggerProviderSharedState
) 🙂
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 suggestion, removed!
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.
Thank you for working on this. 🙂
Which problem is this PR solving?
This change hides the internal methods with a
LoggerProviderSharedState
struct.Short description of the changes
Loggers
should not be responsible for configuration" (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#logger).MultiLogRecordProcessor
with a singleNoopLogRecordProcessor
when no LogRecordProcessor is registered. This can be replaced by a singleNoopLogrecordProcessor
instead.MultiLogRecordProcessor
didn't pass the second argumentcontext
to its processors'onEmit
.Type of change
This un-exposes the internal methods as public so it is a breaking change.
How Has This Been Tested?
Checklist:
Documentation has been updated