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

Align gRPC server status code to span status code #3685

Merged
merged 19 commits into from
Apr 17, 2023

Conversation

FaranIdo
Copy link
Contributor

@FaranIdo FaranIdo commented Apr 7, 2023

@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

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

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.

CHANGELOG.md Outdated Show resolved Hide resolved
FaranIdo and others added 2 commits April 11, 2023 16:56
@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.

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.

Looks great. Thank you for your contribution 👍

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.

5 participants