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 114 commits behind head on main.

Current head 2989fcf differs from pull request most recent head f60e0c3

Please upload 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 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
@MrAlias MrAlias added the bridge: zap Related to the zap bridge label May 17, 2024
MrAlias added a commit that referenced this pull request May 18, 2024
Part of
#5191
and #5586

Pre-work
#5279

---------

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
zailic pushed a commit to zailic/opentelemetry-go-contrib that referenced this pull request May 20, 2024
Part of
open-telemetry#5191
and open-telemetry#5586

Pre-work
open-telemetry#5279

---------

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
pellared added a commit that referenced this pull request May 21, 2024
Part of
#5191

Pre-work
#5279

This PR implements `uInt` methods `objectEncoder`

---------

Co-authored-by: Robert Pająk <pellared@hotmail.com>
pellared pushed a commit that referenced this pull request May 21, 2024
Part of
#5191

Pre-work
#5279

This PR adds skeleton for arrayEncoder
khushijain21 added a commit to khushijain21/opentelemetry-go-contrib that referenced this pull request May 22, 2024
Part of
open-telemetry#5191

Pre-work
open-telemetry#5279

This PR implements `uInt` methods `objectEncoder`

---------

Co-authored-by: Robert Pająk <pellared@hotmail.com>
khushijain21 added a commit to khushijain21/opentelemetry-go-contrib that referenced this pull request May 22, 2024
pellared added a commit that referenced this pull request May 23, 2024
Part of
#5191

Pre-work
#5279

This PR partially implements `Write` method

Co-authored-by: Robert Pająk <pellared@hotmail.com>
pellared pushed a commit that referenced this pull request May 23, 2024
Part of
#5191

Pre-work
#5279

This PR partially implements `arrayEncoder`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bridge: zap Related to the zap bridge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants