-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Refactor componentattribute and move into service #13948
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
base: main
Are you sure you want to change the base?
Refactor componentattribute and move into service #13948
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (86.12%) 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 #13948 +/- ##
==========================================
- Coverage 91.66% 91.59% -0.08%
==========================================
Files 654 657 +3
Lines 42659 42744 +85
==========================================
+ Hits 39104 39151 +47
- Misses 2741 2774 +33
- Partials 814 819 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Regarding code coverage:
CodeCov doesn't take these uses into account as they are in different modules. |
@axw I can't tag you as reviewer but I would appreciate a review from you, notably on how well this fits into the "bring your own SDK" initiative. |
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.
LGTM!
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 @jade-guiton-dd, only minor concerns from me.
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.
Should this be in a test package? I realise it doesn't bring in any extra deps, but I think it would be good hygiene to keep test code separate.
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.
Moved it into a internal/telemetry/telemetrytest
package.
if len(fields) == 1 && fields[0].Type == zapcore.InlineMarshalerType { | ||
if saf, ok := fields[0].Interface.(componentattribute.ScopeAttributesField); ok { |
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 encapsulate this in the componentattribute package? e.g. have a top-level function called func ScopeAttributesFromFields(fields []zapcore.Field) []attribute.KeyValue
. Then the ScopeAttributesField
type could be unexported.
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.
Or keep the core in componentattribute where it was?
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.
Since instantiating an OTel SDK and copying logs to it from the Zap logger is unique to the otelconftelemetry
implementation of telemetry.Factory
, and since the otelzap
dependency this introduces was one of the "problematic" dependencies we wanted to isolate, I'd rather keep this in otelconftelemetry
so Collectors build with another telemetry.Factory
don't have that dependency.
I can definitely encapsulate the logic better however.
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 unexported the ScopeAttributesField
type, and replaced it with a ExtractLogScopeAttributes
function.
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.
Since instantiating an OTel SDK and copying logs to it from the Zap logger is unique to the otelconftelemetry implementation of telemetry.Factory, and since the otelzap dependency this introduces was one of the "problematic" dependencies we wanted to isolate, I'd rather keep this in otelconftelemetry so Collectors build with another telemetry.Factory don't have that dependency.
Ah yeah, thanks for the reminder. Makes sense.
I unexported the ScopeAttributesField type, and replaced it with a ExtractLogScopeAttributes function.
LGTM, thanks!
Description
This PR splits the code handling component attribute injection, currently in
internal/telemetry
andinternal/telemetry/componentattribute
, into three parts:internal/telemetry
;service/telemetry/otelconftelemetry
, since this "copying" behavior is specific to the otelconf-based telemetry provider;service/internal/componentattribute
.The main goals of this split are:
componentattribute
into the serviceI also rewrote a lot of the attribute injection code along the way:
Instead of adding an unexported field in
TelemetrySettings
to store the current set of injected attributes, this set is now stored inside the provider wrappers (which was already partly the case). This allowed me to move theTelemetrySettings
definition back intocomponent
, removing the dependency it had oncomponentattribute
.I completely changed the approach for injection in logs. Instead of a chain of custom core wrappers with a
withAttributeSet
method, injected attributes are now set through the standardzapcore.Core.With
method. By introducing a customObjectMarshaler
, we can make injected attributes act like regular Zap fields, while also allowing special handling in the otelzap wrapper core, which will set them as instrumentation scope attributes instead of log record attributes.If I'm not mistaken, this PR should have no externally-visible changes, so no changelog should be necessary. (Except maybe the
_ struct{}
inTelemetrySettings
, but there was no changelog when we added those elsewhere)Link to tracking issue
Updates #13842
Testing
The large amounts of splitting and refactoring led me to abandon the existing tests, as they would be too hard to adapt. I wrote new tests for the code in
service/telemetry/otelconftelemetry
andservice/internal/componentattribute
, which hopefully should fill that gap appropriately.