opentelemetry-go-contrib icon indicating copy to clipboard operation
opentelemetry-go-contrib copied to clipboard

Logging bridge for logr

Open scorpionknifes opened this issue 1 year ago • 7 comments

Towards #5192

Looking to do benchmarks in a different PR

Notes:

  • slogSink converts any non string value to slog.Any which is handled with fmt.Sprintf("%+v", v) by otelslog. This PR does not do the above and instead opts to use convertValue to get the convert based on type which maybe less performant 🤔
  • convertValue - follows how funcr handle interface{}
  • currently it's designed to convert nested values recursively with 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?

scorpionknifes avatar Apr 03 '24 14:04 scorpionknifes

Codecov Report

Attention: Patch coverage is 99.45055% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.8%. Comparing base (b058429) to head (beae842). Report is 350 commits behind head on main.

Files with missing lines Patch % Lines
bridges/otellogr/logsink.go 99.4% 1 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5357     +/-   ##
=======================================
+ Coverage   65.3%   65.8%   +0.4%     
=======================================
  Files        203     204      +1     
  Lines      12780   12962    +182     
=======================================
+ Hits        8348    8529    +181     
- Misses      4189    4190      +1     
  Partials     243     243             
Files with missing lines Coverage Δ
bridges/otellogr/logsink.go 99.4% <99.4%> (ø)

codecov[bot] avatar Apr 03 '24 15:04 codecov[bot]

Hello, I was wondering what the state of this PR is. @scorpionknifes are you still planning on getting this merged or would you like some help to get it across the finish line? Any out-of-band communication worth documenting here?

mattsains avatar Jun 18 '24 23:06 mattsains

Hello, I was wondering what the state of this PR is. @scorpionknifes are you still planning on getting this merged or would you like some help to get it across the finish line? Any out-of-band communication worth documenting here?

Oops, kinda forgot about this PR, shall try get to it by this weekend 🙏

scorpionknifes avatar Jun 19 '24 03:06 scorpionknifes

@scorpionknifes After chatting with @pellared we think it would be easier for us to review this PR if you split it into smaller ones, like we did for zap and zerolog.

If you want an example, you can look at the various

  • otelzap PRs: https://github.com/open-telemetry/opentelemetry-go-contrib/pulls?q=is%3Apr+is%3Aclosed+otelzap+
  • otelzerolog PRs: https://github.com/open-telemetry/opentelemetry-go-contrib/pulls?q=is%3Apr+author%3AAkhigbeEromo+is%3Aclosed+zerolog

dmathieu avatar Jul 30 '24 08:07 dmathieu

Changing this PR to draft per https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5357#issuecomment-2257740377.

Use this PR as a reference for new PRs.

pellared avatar Jul 30 '24 08:07 pellared

@scorpionknifes, are you able to follow up with creating smaller PRs or can we use your PR to start creating them ourselves (we can still add you as code owner)?

pellared avatar Sep 03 '24 19:09 pellared

I apologize for being slow on this. I’ll look at it over the weekend. I’ll keep you posted! Thanks for your patience

scorpionknifes avatar Sep 04 '24 13:09 scorpionknifes