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

Mark the span as error when the HTTP transporter fails #950

Merged
merged 7 commits into from
Aug 12, 2021

Conversation

slok
Copy link
Contributor

@slok slok commented Aug 10, 2021

Hi! 👋

Currently, the HTTP Transporter is recording the errors, however, is not marking the span as an error, and this is misleading. For example, if the Request fails because there is nobody listening to our client calls we would not see that the request has failed easily (unless an upper layer records the error):

Before:

Screenshot_20210810_070512

After the fix:

Screenshot_20210810_071420

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Nice! I ran into this yesterday when instrumenting a kubernetes webhook myself...

@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Merging #950 (92f55a1) into main (51426c2) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #950     +/-   ##
=======================================
+ Coverage   75.4%   75.6%   +0.1%     
=======================================
  Files         69      69             
  Lines       4458    4460      +2     
=======================================
+ Hits        3363    3372      +9     
+ Misses       952     946      -6     
+ Partials     143     142      -1     
Impacted Files Coverage Δ
instrumentation/net/http/otelhttp/transport.go 90.7% <100.0%> (+11.4%) ⬆️

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
@MrAlias
Copy link
Contributor

MrAlias commented Aug 11, 2021

This looks ready to merge outside of fixing the lint issues:

transport_test.go:313:2: ineffectual assignment to ctx (ineffassign)
	ctx, span := tracer.Start(ctx, "test")
	^

@MrAlias MrAlias added the bug Something isn't working label Aug 11, 2021
Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
@slok
Copy link
Contributor Author

slok commented Aug 12, 2021

Done!

@pellared pellared removed the bug Something isn't working label Aug 12, 2021
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.

Just a few minor comments to the test code. Nice 👍

instrumentation/net/http/otelhttp/transport_test.go Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/transport_test.go Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/transport_test.go Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit be914d3 into open-telemetry:main Aug 12, 2021
@slok slok deleted the slok/fix-http-client-errors branch August 12, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Bugs
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants