Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Add HTTP/W3C text format serializer to Tag propagation component #445

Conversation

mayurkale22
Copy link
Member

Updates #297 This is the HTTP version of of #431.

Based on W3C correlation context format: https://github.com/w3c/correlation-context/blob/master/correlation_context/HTTP_HEADER_FORMAT.md

There will be another PR to use these serializer functions in the HTTP plugin to propagate tags on wire.

@codecov-io
Copy link

codecov-io commented Mar 25, 2019

Codecov Report

Merging #445 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage      95%   95.02%   +0.02%     
==========================================
  Files         147      149       +2     
  Lines        9566     9655      +89     
  Branches      680      683       +3     
==========================================
+ Hits         9088     9175      +87     
- Misses        478      480       +2
Impacted Files Coverage Δ
src/index.ts 100% <0%> (ø) ⬆️
test/test-text-format.ts 100% <0%> (ø)
src/tags/propagation/text-format.ts 94.11% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac0ef77...c056026. Read the comment docs.

*
* OpenCensus uses W3C Correlation Context as the HTTP text format.
* https://github.com/w3c/correlation-context/blob/master/correlation_context/
* HTTP_HEADER_FORMAT.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could you move the HTTP_HEADER_FORMAT.md to the line above so the link is not broken across lines? I think links are a good exception to the 80 char line limit

Copy link
Member Author

Choose a reason for hiding this comment

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

SG, done

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM, I assume there will be another PR adding the Correlation-Context header?

@mayurkale22
Copy link
Member Author

LGTM, I assume there will be another PR adding the Correlation-Context header?

Yes, the actual use of the Correlation-Context header will be done in a follow up PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants