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

Align gRPC server status code to span status code #3685

Merged
merged 19 commits into from
Apr 17, 2023
Merged

Align gRPC server status code to span status code #3685

merged 19 commits into from
Apr 17, 2023

Conversation

FaranIdo
Copy link
Contributor

@FaranIdo FaranIdo commented Apr 7, 2023 •
edited
Loading

@FaranIdo FaranIdo requested a review from a team April 7, 2023 08:47
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 7, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

dmathieu commented Apr 7, 2023

Could you add a changelog entry, as well as tests?

@FaranIdo FaranIdo changed the title Align gRPC server status code to span status code [WIP] Align gRPC server status code to span status code Apr 7, 2023
@FaranIdo
Copy link
Contributor Author

FaranIdo commented Apr 7, 2023

@dmathieu Yes, on it, will add it later

@FaranIdo FaranIdo changed the title [WIP] Align gRPC server status code to span status code Align gRPC server status code to span status code Apr 8, 2023
@FaranIdo
Copy link
Contributor Author

FaranIdo commented Apr 8, 2023

Hi @dmathieu, I have added tests and a changelog entry for the changes I made.
Additionally, I noticed that there were no tests for the StreamServerInterceptor flow, so I went ahead and added them.

@codecov
Copy link

codecov bot commented Apr 10, 2023 •
edited
Loading

Codecov Report

Merging #3685 (d4ab9e5) into main (1870062) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3685     +/-   ##
=======================================
+ Coverage   70.0%   70.1%   +0.1%     
=======================================
  Files        147     147             
  Lines       6966    6973      +7     
=======================================
+ Hits        4880    4892     +12     
+ Misses      1961    1958      -3     
+ Partials     125     123      -2
Impacted Files Coverage Δ
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 85.1% <100.0%> (+1.7%) ⬆️

@dmathieu
Copy link
Member

There still seems to be CI issues.

pellared
pellared reviewed Apr 11, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
FaranIdo and others added 2 commits April 11, 2023 16:56
@FaranIdo @pellared
Update CHANGELOG.md
2b36f98 
fix CR note on the changelog

Co-authored-by: Robert Pająk <pellared@hotmail.com>
@FaranIdo
refactor server tests to use shared assertion methods
5390053
@FaranIdo
Copy link
Contributor Author

@pellared fixed your comments and refactors the stream + unary tests to use shared assert methods

@FaranIdo
Copy link
Contributor Author

@dashpole @pellared @dmathieu - I just wanted to check in and see if there are any additional comments or changes needed on the pull request, or if we can go ahead and proceed with merging it.

@pellared pellared assigned pellared and unassigned pellared Apr 14, 2023
@FaranIdo
Copy link
Contributor Author

@pellared @dmathieu @dashpole - I fixed the CR comments.

@FaranIdo
Copy link
Contributor Author

@MrAlias added all gRPC statuses.
I also did small refactor in the test by removing the name + grpcErr from the serverChecks variable. Now it is much more simple - just grpcCode, span status + span description.

pellared
pellared approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great. Thank you for your contribution 👍

@MrAlias MrAlias merged commit f998e3f into open-telemetry:main Apr 17, 2023
@MrAlias MrAlias mentioned this pull request Apr 28, 2023
Merged
@dragon3 dragon3 mentioned this pull request Nov 3, 2023
Merged
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanyuancheung hanyuancheung hanyuancheung approved these changes

@MrAlias MrAlias MrAlias approved these changes

@pellared pellared pellared approved these changes

@dmathieu dmathieu dmathieu approved these changes

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
untracked
Development

Successfully merging this pull request may close these issues.
5 participants
@FaranIdo @dmathieu @pellared @MrAlias @hanyuancheung