-
Notifications
You must be signed in to change notification settings - Fork 576
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
feat: add LogProcessor to baggagecopy #6277
Conversation
add a Log Record Processor to the baggagecopy package that copies members from baggage and adds them to the log record attributes.
…metry-go-contrib into baggagecopy
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6277 +/- ##
=====================================
Coverage 66.7% 66.7%
=====================================
Files 192 193 +1
Lines 15552 15568 +16
=====================================
+ Hits 10378 10394 +16
Misses 4883 4883
Partials 291 291
|
cc @MikeGoldsmith as code owner. |
Also CC @codeboten |
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
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 looks goof to me.
I've left one suggestion about setting the default filter in the NewLogProcessor
instead of OnEmit
.
// the Baggage found in ctx. Baggage members are filtered by the filter passed | ||
// to NewLogProcessor. | ||
func (processor LogProcessor) OnEmit(ctx context.Context, record *log.Record) error { | ||
filter := processor.filter |
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.
A filter can on be configured when creating the log processor so I think it would be better to set the default of AllowAllMembers in NewLogProcessor
instead of on each call to OnEmit
.
Then OnEmit
doesn't need to worry about checking if filter is nil.
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 can certainly do that, but that does change the expectations slightly.
It adds a possible panic in the Zero value of the LogProcessor
something that we're testing against:
https://github.com/open-telemetry/opentelemetry-go-contrib/pull/6277/files#diff-b4c7636c6f14ae8c68b2f52c7de627b902307b42a5114af060c5f6e16b47c585R120-R134
That test is pulled from the code this mimics from the SpanProcessor.
I can move the nil check to NewLogProcessor
, but then should I just ignore testing against panicing on the OnEmit method?
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 think we want to guard against zero value use. Having the filter check here, similar to the span processor, seems appropriate.
// the Baggage found in ctx. Baggage members are filtered by the filter passed | ||
// to NewLogProcessor. | ||
func (processor LogProcessor) OnEmit(ctx context.Context, record *log.Record) error { | ||
filter := processor.filter |
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 think we want to guard against zero value use. Having the filter check here, similar to the span processor, seems appropriate.
Co-authored-by: Tyler Yahn <[email protected]>
### Added - Add the `WithSource` option to the `go.opentelemetry.io/contrib/bridges/otelslog` log bridge to set the `code.*` attributes in the log record that includes the source location where the record was emitted. (#6253) - Add `ContextWithStartTime` and `StartTimeFromContext` to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`, which allows setting the start time using go context. (#6137) - Set the `code.*` attributes in `go.opentelemetry.io/contrib/bridges/otelzap` if the `zap.Logger` was created with the `AddCaller` or `AddStacktrace` option. (#6268) - Add a `LogProcessor` to `go.opentelemetry.io/contrib/processors/baggagecopy` to copy baggage members to log records. (#6277) - Use `baggagecopy.NewLogProcessor` when configuring a Log Provider. - `NewLogProcessor` accepts a `Filter` function type that selects which baggage members are added to the log record. ### Changed - Transform raw (`slog.KindAny`) attribute values to matching `log.Value` types. For example, `[]string{"foo", "bar"}` attribute value is now transformed to `log.SliceValue(log.StringValue("foo"), log.StringValue("bar"))` instead of `log.String("[foo bar"])`. (#6254) - Upgrade `go.opentelemetry.io/otel/semconv/v1.17.0` to `go.opentelemetry.io/otel/semconv/v1.21.0` in `go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo`. (#6272) - Resource doesn't merge with defaults if a valid resource is configured in `go.opentelemetry.io/contrib/config`. (#6289) ### Fixed - Transform nil attribute values to `log.Value` zero value instead of panicking in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6237) - Transform nil attribute values to `log.Value` zero value instead of panicking in `go.opentelemetry.io/contrib/bridges/otelzap`. (#6237) - Transform nil attribute values to `log.Value` zero value instead of `log.StringValue("<nil>")` in `go.opentelemetry.io/contrib/bridges/otelslog`. (#6246) - Fix `NewClientHandler` so that `rpc.client.request.*` metrics measure requests instead of responses and `rpc.client.responses.*` metrics measure responses instead of requests in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#6250) - Fix issue in `go.opentelemetry.io/contrib/config` causing `otelprom.WithResourceAsConstantLabels` configuration to not be respected. (#6260) - `otel.Handle` is no longer called on a successful shutdown of the Prometheus exporter in `go.opentelemetry.io/contrib/config`. (#6299)
add a Log Record Processor to the baggagecopy package that copies members from baggage and adds them to the log record attributes.
mimics the
baggagecopy
Span Processor, that copies filtered baggage members from the parent span's context and adds them as span attributes, and instead filters baggage members from the log processor OnEmit context and adds them to the log record as attributes.followed the prior art of the existing Span Processor closely to include baggage in logs.