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.

Sign up for GitHub

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

Jump to bottom

otelzap: Basic implementation of Core.Write #5539

Merged
merged 25 commits into from May 15, 2024
Merged

otelzap: Basic implementation of Core.Write #5539

merged 25 commits into from May 15, 2024

Conversation

khushijain21
Copy link
Contributor

@khushijain21 khushijain21 commented May 9, 2024 •
edited

Part of #5191

Pre-work #5279

This PR implements Write method for Core

@khushijain21 khushijain21 requested review from pellared and a team as code owners May 9, 2024 04:27
@khushijain21 khushijain21 changed the title Implement functionality of Core otelzap: Implement functionality of Core May 9, 2024
@khushijain21 khushijain21 marked this pull request as draft May 9, 2024 04:32
@codecov Codecov
Copy link

codecov bot commented May 9, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.2%. Comparing base (e536eba) to head (2db5b6b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5539     +/-   ##
=======================================
+ Coverage   63.1%   63.2%   +0.1%     
=======================================
  Files        193     193             
  Lines      11892   11917     +25     
=======================================
+ Hits        7504    7534     +30     
+ Misses      4171    4166      -5     
  Partials     217     217
Files Coverage Δ
bridges/otelzap/core.go 92.7% <100.0%> (+15.4%) ⬆️

@khushijain21 khushijain21 marked this pull request as ready for review May 9, 2024 04:48
pellared
pellared reviewed May 10, 2024
View reviewed changes
Copy link
Member

@pellared pellared left a comment •
edited

Choose a reason for hiding this comment

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

Can you please change this PR so that it just partially implements Write? Can you remove the code related to Enabled, Check, With?

bridges/otelzap/core.go Outdated Show resolved Hide resolved
bridges/otelzap/core_test.go Outdated Show resolved Hide resolved
khushijain21 and others added 2 commits May 13, 2024 09:17
@khushijain21 @pellared
Update bridges/otelzap/core.go
444be44 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@khushijain21
addressed requested changes
e3900ce
@khushijain21 khushijain21 changed the title otelzap: Implement functionality of Core otelzap: Implement Write method of Core May 13, 2024
@khushijain21 khushijain21 changed the title otelzap: Implement Write method of Core otelzap: Implement write method for Core May 13, 2024
@khushijain21 khushijain21 changed the title otelzap: Implement write method for Core otelzap: Implement write method for Core May 13, 2024
pellared
pellared reviewed May 15, 2024
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Overall LGTM

bridges/otelzap/core.go Outdated Show resolved Hide resolved
bridges/otelzap/core.go Outdated Show resolved Hide resolved
bridges/otelzap/core_test.go Outdated Show resolved Hide resolved
bridges/otelzap/core_test.go Outdated Show resolved Hide resolved
bridges/otelzap/core_test.go Outdated Show resolved Hide resolved
khushijain21 and others added 4 commits May 15, 2024 19:00
@khushijain21 @pellared
Update bridges/otelzap/core.go
f78d702 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@khushijain21 @pellared
Update bridges/otelzap/core.go
5059a86 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@khushijain21
Merge branch 'main' into zapcore2
4571a02
@khushijain21
accomdodate feedback
a3c6c83
@pellared pellared added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label May 15, 2024
@pellared pellared changed the title otelzap: Implement write method for Core otelzap: Basic implementation of Core.Write May 15, 2024
@pellared pellared mentioned this pull request May 15, 2024
Closed
dmathieu
dmathieu reviewed May 15, 2024
View reviewed changes
bridges/otelzap/core.go Show resolved Hide resolved
bridges/otelzap/core_test.go Show resolved Hide resolved
bridges/otelzap/core_test.go Outdated Show resolved Hide resolved
pellared and others added 4 commits May 15, 2024 17:09
@pellared
Update core.go
1d91526
@pellared
Update core.go
fbf0ddd
@khushijain21 @dmathieu
Update bridges/otelzap/core.go
baa33cb 
Co-authored-by: Damien Mathieu <42@dmathieu.com>
@khushijain21 @dmathieu
Update bridges/otelzap/core_test.go
accb80c 
Co-authored-by: Damien Mathieu <42@dmathieu.com>
@pellared pellared requested a review from dmathieu May 15, 2024 15:44
@pellared pellared merged commit 8199709 into open-telemetry:main May 15, 2024
23 checks passed
@pellared
Copy link
Member

Thanks @khushijain21 👍

zailic pushed a commit to zailic/opentelemetry-go-contrib that referenced this pull request May 20, 2024
@khushijain21 @zailic
otelzap: Basic implementation of Core.Write (open-telemetry#5539)
c05740c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@dmathieu dmathieu dmathieu approved these changes

@MrAlias MrAlias MrAlias approved these changes

Assignees
No one assigned
Labels
Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@khushijain21 @pellared @dmathieu @MrAlias