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

otellogr: Fix nil context panic #6527

Merged
merged 22 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7860af2
fix #6509
flc1125 Dec 23, 2024
eb3b273
fix #6509
flc1125 Dec 23, 2024
1a43c6e
fix lint
flc1125 Dec 23, 2024
b8f8641
fix lint
flc1125 Dec 23, 2024
56945df
test: add nil context test case
flc1125 Dec 23, 2024
896c809
test: add nil context test case
flc1125 Dec 23, 2024
5afb0e6
Update CHANGELOG.md
flc1125 Jan 6, 2025
b3df0b1
Merge branch 'main' into fix-issue-6509
flc1125 Jan 6, 2025
16baba2
chore(otellogr): remove custom context support from LogSink
flc1125 Jan 6, 2025
1a8a580
Merge remote-tracking branch 'upstream/main' into fix-issue-6509
flc1125 Jan 6, 2025
119e5f8
refactor(bridges/otellogr): remove context handling from LogSink
flc1125 Jan 6, 2025
599d562
refactor(otellogr): extract context creation in Enabled method for cl…
flc1125 Jan 6, 2025
1d47f7a
revert(test): Roll back to the previous unit test
flc1125 Jan 7, 2025
00fc8dd
revert(test): Roll back to the previous unit test
flc1125 Jan 7, 2025
e74c2c5
Merge branch 'main' into fix-issue-6509
flc1125 Jan 7, 2025
df8fdfb
Merge remote-tracking branch 'upstream/main' into fix-issue-6509
flc1125 Jan 13, 2025
06665d3
test(logsink): add TestLogSinkCtxInInfo to verify context handling in…
flc1125 Jan 13, 2025
b688692
refactor(bridges/otellogr): rename `wantCtxFun` to `wantCtxFunc` in t…
flc1125 Jan 13, 2025
2bfbb28
fix(bridges/otellogr): add context to TestLogSinkCtxInInfo for issue …
flc1125 Jan 13, 2025
db2af98
refactor(bridges/otellogr): suppress linter warnings in logsink_test.go
flc1125 Jan 13, 2025
59ddfac
Merge remote-tracking branch 'upstream/main' into fix-issue-6509
flc1125 Jan 20, 2025
e0c6e07
fix: move context.Background() fix to Unreleased section in CHANGELOG…
flc1125 Jan 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Fixed

- Use `context.Background()` as default context instead of nil in `go.opentelemetry.io/contrib/bridges/otellogr`. (#6527)

<!-- Released section -->
<!-- Don't change this section unless doing release -->

Expand Down
3 changes: 2 additions & 1 deletion bridges/otellogr/logsink.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ func NewLogSink(name string, options ...Option) *LogSink {
logger: c.provider.Logger(name, opts...),
levelSeverity: c.levelSeverity,
opts: opts,
ctx: context.Background(),
}
}

Expand Down Expand Up @@ -240,7 +241,7 @@ func (l *LogSink) Info(level int, msg string, keysAndValues ...any) {

// Init receives optional information about the logr library this
// implementation does not use it.
func (l *LogSink) Init(info logr.RuntimeInfo) {
func (l *LogSink) Init(logr.RuntimeInfo) {
// We don't need to do anything here.
// CallDepth is used to calculate the caller's PC.
// PC is dropped as part of the conversion to the OpenTelemetry log.Record.
Expand Down
34 changes: 34 additions & 0 deletions bridges/otellogr/logsink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,40 @@ func TestLogSink(t *testing.T) {
}
}

// fix https://github.com/open-telemetry/opentelemetry-go-contrib/issues/6509
func TestLogSinkCtxInInfo(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we add test cases to TestLogSink instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Upon checking, the structure of TestLogSink is currently not suitable for my testing scenario. If adjustments are needed, more changes might be required.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Approving. I can look and try refactoring it tomorrow.

rec := logtest.NewRecorder()
ls := NewLogSink("name", WithLoggerProvider(rec))
l := logr.New(ls)
ctx := context.WithValue(context.Background(), "key", "value") // nolint:revive,staticcheck

tests := []struct {
name string
keyValues []any
wantCtxFunc func(ctx context.Context)
}{
{"default", nil, func(ctx context.Context) {
assert.Equal(t, context.Background(), ctx)
}},
{"default with context", []any{"ctx", ctx}, func(ctx context.Context) {
assert.Equal(t, "value", ctx.Value("key"))
}},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defer rec.Reset()

l.Info("msg", tt.keyValues...)
require.Len(t, rec.Result(), 1)

got := rec.Result()[0]
require.Len(t, got.Records, 1)
tt.wantCtxFunc(got.Records[0].Context())
})
}
}

func TestLogSinkEnabled(t *testing.T) {
enabledFunc := func(ctx context.Context, param log.EnabledParameters) bool {
return param.Severity == log.SeverityInfo
Expand Down
Loading