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

feat(plugin-http): sync. specs for statuscode #719

Merged

Conversation

OlivierAlbertini
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • Synchronise HTTP status code with CanonicalCode according to specs.

I try to use the benchmark system we have, see my sample code here : https://gist.github.com/OlivierAlbertini/9505254b713b9db5f60168ed3fa4e437

But the result is a bit random (on my macbook pro 2016). I would like to know which one is faster. Any insight ?
If we want to share this code with the web, I think we should discuss it and resolve it in a separate PR.

@codecov-io
Copy link

codecov-io commented Jan 22, 2020

Codecov Report

Merging #719 into master will decrease coverage by 1.74%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
- Coverage   92.75%   91.01%   -1.75%     
==========================================
  Files         228      225       -3     
  Lines       10451    10401      -50     
  Branches      941      953      +12     
==========================================
- Hits         9694     9466     -228     
- Misses        757      935     +178
Impacted Files Coverage Δ
...lemetry-plugin-http/test/functionals/utils.test.ts 99.37% <100%> (+0.02%) ⬆️
packages/opentelemetry-plugin-http/src/utils.ts 97.26% <100%> (-0.66%) ⬇️
...y-plugin-http/test/functionals/http-enable.test.ts 96.21% <100%> (ø) ⬆️
packages/opentelemetry-tracing/src/version.ts 0% <0%> (ø) ⬆️
...s/opentelemetry-scope-zone-peer-dep/src/version.ts 0% <0%> (ø) ⬆️
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (ø) ⬆️
...y-exporter-collector/test/common/transform.test.ts 0% <0%> (ø) ⬆️
packages/opentelemetry-core/test/utils/url.test.ts 50% <0%> (-50%) ⬇️
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
... and 129 more

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

lgtm

@mayurkale22 mayurkale22 added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) labels Jan 22, 2020
test: fix and add tests
closes open-telemetry#642

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan merged commit 53dda64 into open-telemetry:master Jan 28, 2020
@OlivierAlbertini OlivierAlbertini deleted the feature/specs-statuscode branch February 5, 2020 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin-http] update parseResponseStatus from new specs
7 participants