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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Logging bridge for logr #5357
base: main
Are you sure you want to change the base?
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
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 made a quick review.
bridges/otellogr/logsink.go
Outdated
if len(l.name) > 0 { | ||
l.name += "/" | ||
} | ||
l.name += name | ||
return &l |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be an codeowner?
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.
Is the requirement to be a member?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happy to sponsor you as well.
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.
Appreciate it 馃檹 thanks
1da0e2f
to
9d75ae7
Compare
9d75ae7
to
334ce09
Compare
if l.name != "" { | ||
record.AddAttributes(log.String(nameKey, l.name)) | ||
} |
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.
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.Any
which is handled with fmt.Sprintf("%+v", v) by otelslog. This PR does not do the above and instead opts to useconvertValue
to 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?