otellogr: Implement Logsink.Info() and conversions #6111
Conversation
| // The Level is transformed by using the [WithLevels] option. If the option is | ||
| // not provided, all logr levels will be mapped to [log.SeverityInfo]. | ||
| // The levels are mapped in order, with the first level in the list corresponding | ||
| // to the logr level 0, the second level corresponding to logr level 1, and so on. | ||
| // If the logr level is greater than the number of levels provided, it will be | ||
| // mapped to [log.SeverityInfo]. |
There was a problem hiding this comment.
Trying out a different level transformation
Original attempt https://github.com/scorpionknifes/opentelemetry-go-contrib/blob/beae842225fd1a14f5a935cd4e419b0e519e3ac9/bridges/otellogr/example_test.go#L23-L33
Open to suggestions
There was a problem hiding this comment.
In scope of this PR I suggest removing level transformation and leave it to a separate PR.
Personally, I preferred the previous design as it was more flexible.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6111 +/- ##
=======================================
+ Coverage 65.7% 66.1% +0.3%
=======================================
Files 204 205 +1
Lines 13085 13209 +124
=======================================
+ Hits 8607 8738 +131
+ Misses 4215 4208 -7
Partials 263 263
|
pellared
added
the
Skip Changelog
| // The logr records are converted to OpenTelemetry [log.Record] in the following | ||
| // way: | ||
| // | ||
| // - Time is set as the current time of conversion. |
There was a problem hiding this comment.
Is this true?
| // - Time is set as the current time of conversion. |
| // The Level is transformed by using the [WithLevels] option. If the option is | ||
| // not provided, all logr levels will be mapped to [log.SeverityInfo]. | ||
| // The levels are mapped in order, with the first level in the list corresponding | ||
| // to the logr level 0, the second level corresponding to logr level 1, and so on. | ||
| // If the logr level is greater than the number of levels provided, it will be | ||
| // mapped to [log.SeverityInfo]. |
There was a problem hiding this comment.
In scope of this PR I suggest removing level transformation and leave it to a separate PR.
Personally, I preferred the previous design as it was more flexible.
| ls := NewLogSink("name", WithLoggerProvider(rec)) | ||
| lsWithName := ls.WithName("test") | ||
| require.NotEqual(t, ls, lsWithName) | ||
| require.Equal(t, lsWithName, ls.WithName("test")) |
There was a problem hiding this comment.
I strongly prefer if we assert the exported records (behavior/user output) instead of testing the internals.
| for _, tt := range []struct { | ||
| name string | ||
| f func(*logr.Logger) | ||
| wantLoggerCount int |
| } | ||
|
|
||
| func TestConvertValue(t *testing.T) { | ||
| now := time.Now() |
There was a problem hiding this comment.
Why not a constant value as in other places?
| name string | ||
| f func(*logr.Logger) | ||
| wantLoggerCount int | ||
| wantRecords []wantRecord |
There was a problem hiding this comment.
We are always emitting one record. No need for a slice here.
| last := len(rec.Result()) - 1 | ||
|
|
||
| assert.Len(t, rec.Result()[last].Records, len(tt.wantRecords)) | ||
| for i, record := range rec.Result()[last].Records { |
There was a problem hiding this comment.
We should also assert the Name, Version, and SchemeURL of ScopeRecords
This PR implements a few methods for otellogr package
TODO remaining
Please add skip changelog thx 🙏
Part of #5192