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

Use server span when determining status from code #1973

Merged
merged 7 commits into from Apr 7, 2022
Merged

Use server span when determining status from code #1973

merged 7 commits into from Apr 7, 2022

Conversation

kitschysynq
Copy link
Contributor

Switches to SpanStatusFromHTTPStatusCodeAndSpanKind with a span kind
of SpanKindServer so that 4xx status codes don't get labelled as
errors. Since these codes represent client errors, they make it
difficult to differentiate bad server behavior from bad client behavior.

Switches to `SpanStatusFromHTTPStatusCodeAndSpanKind` with a span kind
of `SpanKindServer` so that 4xx status codes don't get labelled as
errors. Since these codes represent client errors, they make it
difficult to differentiate bad server behavior from bad client behavior.
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Can you please add a test and CHANGELOG entry for this new behavior?

@kitschysynq
Copy link
Contributor Author

Sure thing, thanks for looking!

@kitschysynq
Copy link
Contributor Author

I pushed the CHANGELOG entry but it's not clear to me how to test the result. Do you have any advice for checking the state of a span after the handler and middleware have returned?

@MrAlias
Copy link
Contributor

MrAlias commented Mar 22, 2022

I pushed the CHANGELOG entry but it's not clear to me how to test the result. Do you have any advice for checking the state of a span after the handler and middleware have returned?

You'll probably need to create a test case similar to this:

The test module is used to check integration and will be able to determine the final span state the default SDK will produce.

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #1973 (353f3c5) into main (20f04ef) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1973   +/-   ##
=====================================
  Coverage   69.4%   69.4%           
=====================================
  Files        135     135           
  Lines       6114    6114           
=====================================
+ Hits        4244    4247    +3     
+ Misses      1742    1739    -3     
  Partials     128     128           
Impacted Files Coverage Δ
...trumentation/github.com/gorilla/mux/otelmux/mux.go 86.2% <100.0%> (+3.7%) ⬆️

Adds a test to verify that the span created by a request that returns
status "not found" is not an error span.
@kitschysynq
Copy link
Contributor Author

Thanks for the info. Added that test and caught the branch up with main.

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.

None yet

4 participants