Skip to content
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

otelslog: Add WithSource option #6253

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

Jesse0Michael
Copy link
Contributor

@Jesse0Michael Jesse0Michael commented Oct 15, 2024

Add an slogbridge option to include the source file location in the log attributes.

Resolves #6244

@Jesse0Michael Jesse0Michael requested review from MrAlias, pellared and a team as code owners October 15, 2024 04:07
Copy link

linux-foundation-easycla bot commented Oct 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

bridges/otelslog/handler_test.go Outdated Show resolved Hide resolved
bridges/otelslog/handler_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@pellared
Copy link
Member

Side note from CONTRIBUTING.mD:

Avoid rebasing and force-pushing to your branch to facilitate reviewing the pull request. Rewriting Git history makes it difficult to keep track of iterations during code review. All pull requests are squashed to a single commit upon merge to main.

@Jesse0Michael
Copy link
Contributor Author

Jesse0Michael commented Oct 16, 2024

@pellared Sorry about the rebase/push! And thanks for pointing me to the `recorder. I have a better understanding on how it's tested now.

I updated the test to be a new testCase in TestSLogHandler

To do that I made it possible to specify options in the test case to apply to Handler.

It also requires some expected values to be created at the top of the test to be used in the checks.

If those changes are not ideal, let me know and I'll move it back to its own test using the recorder

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.2%. Comparing base (13536dd) to head (066b5a3).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6253   +/-   ##
=====================================
  Coverage   67.2%   67.2%           
=====================================
  Files        191     191           
  Lines      12642   12657   +15     
=====================================
+ Hits        8497    8510   +13     
- Misses      3855    3856    +1     
- Partials     290     291    +1     
Files with missing lines Coverage Δ
bridges/otelslog/handler.go 96.7% <100.0%> (+0.2%) ⬆️

... and 1 file with indirect coverage changes

@pellared
Copy link
Member

@Jesse0Michael, no worries. I find this PR very good and helpful. Thanks a lot for your contribution.

@pellared pellared added this to the v1.32.0 milestone Oct 16, 2024
@pellared pellared added bridge: slog Related to the slog bridge enhancement New feature or request labels Oct 16, 2024
@pellared pellared changed the title feat(otelslog): add WithSource option otelslog: Add WithSource option Oct 16, 2024
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6253 (comment) needs being addressed

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@pellared
Copy link
Member

@dmathieu PTAL

@pellared
Copy link
Member

I plan to merge it tomorrow morning. I want others to give at least one more day to review.

@pellared pellared merged commit 1c56a7c into open-telemetry:main Oct 18, 2024
25 checks passed
@Jesse0Michael Jesse0Michael deleted the slogbridge-withsource branch October 18, 2024 15:02
pellared added a commit that referenced this pull request Nov 8, 2024
### 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)
pellared added a commit that referenced this pull request Dec 11, 2024
Follow semantic conventions more strictly. See the following thread:
open-telemetry/semantic-conventions#1624 (comment)

Follows
#6253

There is no noticeable performance overhead:

```
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/contrib/bridges/otelslog
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
                               │   old.txt    │               new.txt               │
                               │    sec/op    │   sec/op     vs base                │
Handler/(WithSource).Handle-16   260.4n ± 21%   234.0n ± 5%  -10.14% (p=0.035 n=10)

                               │  old.txt   │            new.txt             │
                               │    B/op    │    B/op     vs base            │
Handler/(WithSource).Handle-16   248.0 ± 0%   248.0 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

                               │  old.txt   │            new.txt             │
                               │ allocs/op  │ allocs/op   vs base            │
Handler/(WithSource).Handle-16   2.000 ± 0%   2.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bridge: slog Related to the slog bridge enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

otelslog: WithSource
4 participants