-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Inject component-identifying scope attributes #12617
base: main
Are you sure you want to change the base?
Inject component-identifying scope attributes #12617
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (91.51%) 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 #12617 +/- ##
==========================================
- Coverage 91.66% 91.54% -0.13%
==========================================
Files 479 484 +5
Lines 26355 26497 +142
==========================================
+ Hits 24159 24256 +97
- Misses 1733 1777 +44
- Partials 463 464 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
return logger.WithOptions(zap.WrapCore(func(c zapcore.Core) zapcore.Core { | ||
sc, ok := c.(serviceCore) | ||
if !ok { | ||
// Logger does not use OTel under the hood, cannot change Logger parameters |
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.
Does it make sense to add a log about this?
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 think so. This is not an error condition, as logs are only exported with OTel if some processors are set in the config. When that isn't the case, we get a pure Zap-based console logger, and there is no Logger
to set attributes on.
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.
As discussed in the Collector stability meeting, I'm working on adding the attributes to the console logger output as well, which I think will require a core wrapper in all cases. I'll try to add a warn-level log if the wrapper isn't found.
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.
In the end it was a bit tricky so I didn't do it, but I'm thinking relying on our tests might be enough? Since we have tests in componentattribute
checking that attributes are set properly after multiple calls to ZapLoggerWithAttributes
, and a test in service/telemetry
checking that the instantiated logger is the expected type
Co-authored-by: Pablo Baeyens <[email protected]>
|
||
var _ coreWithAttributes = consoleCoreWithAttributes{} | ||
|
||
func NewConsoleCoreWithAttributes(c zapcore.Core, attrs attribute.Set) zapcore.Core { |
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 understand the 'console' in the name of this function. what does it refer to?
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 the Core used for writing to stdout (ie. "the console"), with the component attributes inserted as standard Zap attributes. This is by opposition with the "OTel Tee" core, which tees a console core with an OTel output core, which uses scope attributes. I can rename it if you think the terminology is unclear.
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.
It seems to me that the function is agnostic to where the core is actually writing and console is just one of the use cases. I would have understood it better if console was not in the name and we clarified the intended use cases for each constructor in godoc comments instead
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.
That makes sense. I'm not sure what to call it if not based on the use case though... Any ideas? zapCoreWithAttributes
? attributesAsFieldsWrapper
vs. attributesInScopeWrapper
? (although the latter also has the Tee feature, so that might be confusing)
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 added some comments to the different core wrappers to clarify what they do and the intended use, hopefully that should alleviate the questionable naming.
Description
Fork of #12384 to showcase how component attributes can be injected into scope attributes instead of log/metric/span attributes. See that PR for more context.
To see the diff from the previous PR, filter changes starting from the "Prototype using scope attributes" commit.
Link to tracking issue
Resolves #12217
Also incidentally resolves #12213 and resolves #12117
Testing
I updated the existing tests to check for scope attributes, and did some manual testing with a debug exporter to check that the scope attributes are added/removed properly.