Logging bridge for logr #5357
Logging bridge for logr #5357
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5357 +/- ##
=======================================
+ Coverage 62.3% 62.9% +0.5%
=======================================
Files 189 190 +1
Lines 11575 11755 +180
=======================================
+ Hits 7219 7398 +179
- Misses 4146 4147 +1
Partials 210 210
|
scorpionknifes
requested review from
MrAlias,
MadVikingGod,
pellared and
dashpole
as code owners
bridges/otellogr/logsink.go
Outdated
| // nameKey is used to log the `WithName` values as an additional attribute. | ||
| nameKey = "logger" | ||
| // errKey is used to log the error parameter of Error as an additional attribute. | ||
| errKey = "err" |
There was a problem hiding this comment.
We should use semantic conventions for this, err is too go-specific.
Logs have the notion of exception attributes, but not errors. Maybe that's what we should rely on?
There was a problem hiding this comment.
Actually, thinking about it, I wonder if we shouldn't suggest adding an error namespace to semantic conventions.
I have started a semantic conventions discussion about this: open-telemetry/semantic-conventions#921
There was a problem hiding this comment.
I propose to rename it to error for now. I think this should not block this PR given that this bridge would be versioned as experimental.
Co-authored-by: Damien Mathieu <42@dmathieu.com>
bridges/otellogr/logsink.go
Outdated
| // - Context is not propagated to the OpenTelemetry log record, as logr does | ||
| // not provide a way to access it. |
There was a problem hiding this comment.
Can we handle it if someone passes context as an attributes?
This is what I have done in https://github.com/pellared/opentelemetry-go/blob/147762ffb5c4394417b3f973274cc570723db79b/log/internal/logr.go.
bridges/otellogr/logsink.go
Outdated
| if len(l.name) > 0 { | ||
| l.name += "/" | ||
| } | ||
| l.name += name | ||
| return &l |
There was a problem hiding this comment.
We should rather create a new LogSink with a new l.logger that has the name appended to the previous logger. See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md?plain=1#L72-L74
| @@ -22,6 +22,7 @@ | |||
|
|
|||
| CODEOWNERS @MrAlias @MadVikingGod @pellared @dashpole | |||
|
|
|||
| bridges/otellogr/ @open-telemetry/go-approvers | |||
There was a problem hiding this comment.
Is the requirement to be a member?
There was a problem hiding this comment.
No it is not required.
However, if you want to be I can be your sponsor per https://github.com/pulls?q=author%3Ascorpionknifes+org%3Aopen-telemetry+is%3Apr+is%3Amerged+
You would just need to find a second sponsor.
There was a problem hiding this comment.
I'd be happy to sponsor you as well.
There was a problem hiding this comment.
Appreciate it 馃檹 thanks
scorpionknifes
requested review from
Aneurysm9 and
hanyuancheung
as code owners
scorpionknifes
force-pushed
the
feature/impl-otellogr
branch
2 times, most recently
from
1da0e2f
to
9d75ae7
Compare
scorpionknifes
force-pushed
the
feature/impl-otellogr
branch
from
9d75ae7
to
334ce09
Compare
| if l.name != "" { | ||
| record.AddAttributes(log.String(nameKey, l.name)) | ||
| } |
There was a problem hiding this comment.
Instead of doing this the WithName method should create a new l.logger. The nameKey should not be needed.
Resolves #5192
Looking to do benchmarks in a different PR
Notes:
slog.Anywhich is handled with fmt.Sprintf("%+v", v) by otelslog. This PR does not do the above and instead opts to useconvertValueto get the convert based on type which maybe less performant 馃convertValue- follows how funcr handle interface{}convertValue, is there a max depth for converted attributes?kvBuffer- is used in otelslog, I propose to move kvBuffer to internal pkg and reused it here too?