-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Introduce component logger with appropriate attributes #12259
base: main
Are you sure you want to change the base?
Conversation
ac84dde
to
5efff9d
Compare
8845f3e
to
e8565de
Compare
8ebe64f
to
0cdc5de
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (90.32%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12259 +/- ##
==========================================
- Coverage 91.35% 91.32% -0.03%
==========================================
Files 467 467
Lines 25761 25780 +19
==========================================
+ Hits 23533 23543 +10
- Misses 1810 1819 +9
Partials 418 418 ☔ View full report in Codecov by Sentry. |
ddc480f
to
39415d7
Compare
39415d7
to
a9d57b9
Compare
type Core struct { | ||
zapcore.Core | ||
from *zap.Logger | ||
attrs attribute.Set | ||
} |
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.
Looking at usages (outside the service) the only think needed is Without(keys ...string) *zap.Logger
Can we do some golang magic, and actually public expose only an interface (rest of the implementation is in otelcol/service)?
The ideal scenario may be something that extend Logger, not sure if that works because Logger is a struct.
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.
Here is where I got #12289
I don't have to expose public any interface. We will still need to expose the fields keys not sure where though.
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.
I don't have to expose public any interface
I'm trying to incorporate your solution to simplify mine, but one consideration overlooked is that in practice we pass different kinds of loggers into tests, so we must make a type assertion to check for the Without
method. If we're doing this, I think we need to at least give a name the interface so that components do not need to assert against an anonymous interface. The alternative would be to enforce somehow that tests use the same logger, but this is impractical IMO.
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.
Similarly, for tests that do want to validate the functionality of the shared logger, they need to be able to instantiate a logger, so at least then you need to expose a testing logger which is very similar to the actual logger.
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.
I was able to reduce the surface area of componentattribute
but still think we need a constructor for the logger
return zap.New(&Core{ | ||
Core: withAttributes.Core(), | ||
from: l.from, | ||
attrs: attribute.NewSet(newAttrs...), | ||
}) |
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.
With this people lose other settings (options of the logger). You need to use logger.WithOptions(zap.WrapCore(...){})
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.
I've reworked logger creation to use WithOptions
attrs attribute.Set | ||
} | ||
|
||
func NewLogger(from *zap.Logger, attrs *attribute.Set) *zap.Logger { |
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.
I'm not sure how to avoid exposing this. It's used in component tests, and also in the graph when instantiating the loggers for components.
6eb8f93
to
1f629b7
Compare
type loggerCore interface { | ||
zapcore.Core | ||
Without(fields ...string) *zap.Logger | ||
} |
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.
This is defined both here and internally in component
in order to avoid exposing it.
Implements the logger described in #12217
Alternative to #12057
Resolves #11814
component/componentattribute
:zapcore.Core
which can remove attributes from the root loggerservice
:componentattribute
otlpreceiver
:componentattribute
to removeotelcol.signal
attribute from loggermemorylimiter
:componentattribute
to removeotelcol.signal
,otelcol.pipeline.id
andotelcol.component.id
attributes from logger