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

otelgrpc: Fix NewServerHandler to correctly set span status #4587

Merged
merged 5 commits into from
Nov 21, 2023
Merged

otelgrpc: Fix NewServerHandler to correctly set span status #4587

merged 5 commits into from
Nov 21, 2023

Conversation

scorpionknifes
Copy link
Member

fix #4586

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Nov 17, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@scorpionknifes scorpionknifes marked this pull request as ready for review November 19, 2023 23:05
@scorpionknifes scorpionknifes requested a review from a team November 19, 2023 23:05
pellared

This comment was marked as outdated.

Sign in to view

@pellared pellared dismissed their stale review November 20, 2023 10:58

I was too quick with my review...

pellared
pellared reviewed Nov 20, 2023
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 looks good 👍 Few tiny comments

instrumentation/google.golang.org/grpc/otelgrpc/config.go Outdated Show resolved Hide resolved
instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go Outdated Show resolved Hide resolved
instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go Outdated Show resolved Hide resolved
pellared
pellared reviewed Nov 20, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@pellared pellared changed the title otelgrpc: Fix serverhandler setting span as error for all grpc errors otelgrpc: Fix NewServerHandler to correctly set span status Nov 20, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 20, 2023 •
edited
Loading

Codecov Report

Merging #4587 (36f8513) into main (ba19074) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4587   +/-   ##
=====================================
  Coverage   80.9%   80.9%           
=====================================
  Files        150     150           
  Lines      10348   10355    +7     
=====================================
+ Hits        8375    8382    +7     
  Misses      1831    1831           
  Partials     142     142
Files Coverage Δ
...n/google.golang.org/grpc/otelgrpc/stats_handler.go 93.1% <100.0%> (+0.3%) ⬆️

pellared
pellared approved these changes Nov 20, 2023
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.

Thank you 🥇

@pellared pellared merged commit 698903f into open-telemetry:main Nov 21, 2023
21 checks passed
@MadVikingGod MadVikingGod mentioned this pull request Jan 18, 2024
Merged
@scorpionknifes scorpionknifes mentioned this pull request Apr 24, 2024
Closed
6 tasks
@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

@pellared pellared pellared approved these changes

@dashpole dashpole dashpole approved these changes

@dmathieu dmathieu dmathieu approved these changes

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

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

Successfully merging this pull request may close these issues.

otelgrpc: server stats handler sets all errors to error=true
4 participants
@scorpionknifes @dmathieu @dashpole @pellared