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

Logging bridge for Zap #5279

Draft
wants to merge 55 commits into
base: main
Choose a base branch
from
Draft

Conversation

khushijain21
Copy link
Contributor

@khushijain21 khushijain21 commented Mar 14, 2024

Part of #5191

Some Notes:
Basic Structure - Zapcore implements
- Check - determines whether the supplied Entry should be logged
- Write
- Enabled
- Sync - we don't use buffer to write - so no code here
- With - returns a child core with provided context

This also implements zapcore.ObjectEncoder and zapcore.ArrayEncoder which decides how we would encode provided fields to log attrs - (the user can also pass their custom encoder)
Some Notes about type mapping:

  • All Uint types are converted to Int64
  • All complex types are converted to equiv string
  • Everything else which uses zap.Any to pass context and if does not match supported primitive types - is handled by AddReflected method which converts the field to JSON

@khushijain21 khushijain21 requested a review from a team as a code owner March 14, 2024 13:36
@khushijain21 khushijain21 marked this pull request as draft March 14, 2024 13:37
@khushijain21 khushijain21 changed the title Logging bridge for Zap WIP: Logging bridge for Zap Mar 14, 2024
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 71.92982% with 80 lines in your changes are missing coverage. Please review.

Project coverage is 62.1%. Comparing base (30ed923) to head (2989fcf).
Report is 8 commits behind head on main.

❗ Current head 2989fcf differs from pull request most recent head f60e0c3. Consider uploading reports for the commit f60e0c3 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5279     +/-   ##
=======================================
- Coverage   62.3%   62.1%   -0.3%     
=======================================
  Files        189     190      +1     
  Lines      11575   11673     +98     
=======================================
+ Hits        7221    7256     +35     
- Misses      4145    4205     +60     
- Partials     209     212      +3     
Files Coverage Δ
bridges/otelzap/config.go 76.6% <76.6%> (ø)
bridges/otelzap/core.go 89.0% <89.0%> (ø)
bridges/otelzap/test_helper.go 73.7% <73.7%> (ø)
bridges/otelzap/encoder.go 61.5% <61.5%> (ø)

... and 7 files with indirect coverage changes

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Good start

bridges/zap/go.mod Outdated Show resolved Hide resolved
bridges/zap/go.mod Outdated Show resolved Hide resolved
bridges/zap/version.go Outdated Show resolved Hide resolved
bridges/zap/version.go Outdated Show resolved Hide resolved
bridges/zap/zap_encoder.go Outdated Show resolved Hide resolved
bridges/zap/zap_logger.go Outdated Show resolved Hide resolved
bridges/zap/zap_logger.go Outdated Show resolved Hide resolved
bridges/zap/zap_logger.go Outdated Show resolved Hide resolved
bridges/zap/zap_test.go Outdated Show resolved Hide resolved
bridges/zap/zap_logger.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Mar 22, 2024

This module will need to be added to the project versions.yaml under the unreleased section.

@pellared pellared added Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG and removed Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG labels Mar 26, 2024
@khushijain21 khushijain21 changed the title WIP: Logging bridge for Zap Logging bridge for Zap Apr 5, 2024
@pellared
Copy link
Member

pellared commented Apr 8, 2024

https://github.com/open-telemetry/opentelemetry-go-contrib/actions/runs/8595139722/job/23549314101?pr=5279

Please run make to check for CI build errors.

bridges/otelzap/core.go Outdated Show resolved Hide resolved
bridges/otelzap/core.go Outdated Show resolved Hide resolved
bridges/otelzap/encoder.go Outdated Show resolved Hide resolved
bridges/otelzap/core.go Outdated Show resolved Hide resolved
@khushijain21 khushijain21 changed the title Logging bridge for Zap WIP: Logging bridge for Zap Apr 8, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Why do we even need to implement the encoders if the only think we have to provide is a zapcore.Core?

bridges/otelzap/core.go Outdated Show resolved Hide resolved
type embeddedLogger = embedded.Logger // nolint:unused // Used below.

// recorder records all [log.Record]s it is ased to emit.
type recorder struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use https://pkg.go.dev/go.opentelemetry.io/otel/log@v0.1.0-alpha.0.20240409140646-648b40eae158/logtest instead?

You can get it via go get go.opentelemetry.io/otel/log@v0.1.0-alpha.0.20240409140646-648b40eae158.

@khushijain21 khushijain21 changed the title WIP: Logging bridge for Zap Logging bridge for Zap Apr 22, 2024
@pellared
Copy link
Member

pellared commented May 6, 2024

Changing this to a draft PR as I assume this is a "reference/design PR".

@pellared pellared marked this pull request as draft May 6, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants