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

Fix Http Status Code with Otel Bridge #3265

Merged
merged 5 commits into from Oct 13, 2022

Conversation

alanprot
Copy link
Contributor

@alanprot alanprot commented Oct 7, 2022

HttpStatus code is not working correctly as status code is set as int16 but the otel bridge does not expect int16 attributes. As result, the status code is set as string.

See: https://github.com/opentracing-contrib/go-stdlib/blob/master/nethttp/server.go#L143-L145

This is problematic as the exporters (xray for instance) expects the status code to be set as an int and defaults to 0 if its not.

See: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/6dc71bd739b4eaa407a9a596dc57ca4884e6313e/exporter/awsxrayexporter/internal/translator/http.go#L57-L59

Before:
Screen Shot 2022-10-06 at 9 04 11 PM

After:

Screen Shot 2022-10-06 at 9 02 46 PM

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #3265 (68c53c0) into main (a8b9ddc) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3265   +/-   ##
=====================================
  Coverage   77.7%   77.7%           
=====================================
  Files        163     163           
  Lines      11227   11235    +8     
=====================================
+ Hits        8730    8738    +8     
  Misses      2299    2299           
  Partials     198     198           
Impacted Files Coverage Δ
bridge/opentracing/bridge.go 63.4% <100.0%> (+0.5%) ⬆️

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.

Please add a changelog entry.

@alanprot
Copy link
Contributor Author

alanprot commented Oct 7, 2022

Please add a changelog entry.

done! thx!

@alanprot
Copy link
Contributor Author

Hi..

Is anything missing to merge this pr ?

CHANGELOG.md Outdated Show resolved Hide resolved
alanprot and others added 2 commits October 12, 2022 15:38
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MadVikingGod MadVikingGod merged commit 84e28fd into open-telemetry:main Oct 13, 2022
@alanprot alanprot deleted the fix-status branch October 13, 2022 17:52
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

6 participants