HTTP Propagation: set `Correlation-Context` header #567

mayurkale22 · 2019-05-30T21:51:50Z

Continuation of #445 and Fixes #297

Specs: https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md#over-http

codecov-io · 2019-05-30T21:56:01Z

Codecov Report

Merging #567 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #567      +/-   ##
=========================================
+ Coverage    95.3%   95.3%   +<.01%     
=========================================
  Files         148     148              
  Lines       10590   10656      +66     
  Branches      746     749       +3     
=========================================
+ Hits        10093   10156      +63     
- Misses        497     500       +3

Impacted Files	Coverage Δ
src/stackdriver-monitoring.ts	`77.02% <0%> (-2.71%)`	⬇️
src/http.ts	`90.3% <0%> (-0.06%)`	⬇️
src/binary-format.ts	`100% <0%> (ø)`	⬆️
test/test-http.ts	`98.86% <0%> (+0.24%)`	⬆️

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 039c695...958c9e3. Read the comment docs.

packages/opencensus-instrumentation-http/src/http.ts

songy23 · 2019-05-31T15:45:11Z

packages/opencensus-instrumentation-http/src/http.ts

+   *     retrieved.
+   */
+  static getTagContext(headers: IncomingHttpHeaders): TagMap|null {
+    const contextValue = (headers[CORRELATION_CONTEXT.toLocaleLowerCase()] ||


Why toLocaleLowerCase?

AFAIK Node core automatically converts all HTTP headers to lowercase. Without much frills, I easily found the location where outgoing HTTP headers are converted to lowercase.

@draffensperger correct me if this statement is wrong.

What would you think about just making the constant itself be in lower case so you don't need to convert it here along with a comment that Node converts headers to lower case?

I'm not super familiar with how headers in Node are handled. Could this be related? nodejs/node-v0.x-archive#1954
How are headers for trace context handled? Do we assume those are lowercase?

TraceContext headers are defined in lower case only. As per official documentation -> In order to increase interoperability across multiple protocols and encourage successful integration by default it is recommended to keep the header name lower case. Platforms and libraries MUST expect header name in any casing and SHOULD send header name in lower case.

OK, I guess I'm a little confused by the code then, to me it looks like you are looking for the header key in lower case rather than converting all the incoming headers to lower case to compare them?

Or am I missing something here?

TraceContext headers are defined in lower case only.

That's for TraceContext. I don't think it's the case for Correlation-Context, per https://github.com/w3c/correlation-context/blob/master/correlation_context/HTTP_HEADER_FORMAT.md#examples-of-http-headers.

Phrased a different way, how do you know that headers: IncomingHttpHeaders here will always have its values in lower case? Could they ever be in upper-case, and if so, should you just convert them to lower case to make sure before comparing them to a lower-cased header?

how do you know that headers: IncomingHttpHeaders here will always have its values in lower case? Could they ever be in upper-case?

Not 100% sure on this, perhaps that's why we use both the variant of user-agent key. I think it is safe bet to compare both the variant until we don;t know the complete picture.

Makes sense. Maybe you can just making both the incoming header and the constant be lower case and compare those?

I'm going to approve this, and if this comes up as a bug in the future, it should be fixed.

songy23 · 2019-05-31T15:47:32Z

packages/opencensus-instrumentation-http/test/test-http.ts

@@ -290,6 +303,60 @@ describe('HttpPlugin', () => {
      });
    });

+    it('should create a child span for GET requests with tag context', () => {


I'm not sure if I understand these tests - why test spans? Same for other tests.

draffensperger · 2019-06-08T00:39:00Z

packages/opencensus-instrumentation-http/src/http.ts

+   *     retrieved.
+   */
+  static getTagContext(headers: IncomingHttpHeaders): TagMap|null {
+    const contextValue = (headers[CORRELATION_CONTEXT.toLocaleLowerCase()] ||


I'm going to approve this, and if this comes up as a bug in the future, it should be fixed.

mayurkale22 requested review from draffensperger and justindsmith as code owners May 30, 2019 21:51

googlebot added the cla: yes label May 30, 2019

mayurkale22 requested a review from songy23 May 31, 2019 03:55

songy23 reviewed May 31, 2019

View reviewed changes

mayurkale22 force-pushed the Correlation-Contex branch from f19c2b4 to 958c9e3 Compare May 31, 2019 17:39

mayurkale22 added 2 commits June 7, 2019 11:08

set Correlation-Context header

5b9283e

Rename TagContext -> TagMap

4bf8ba1

mayurkale22 force-pushed the Correlation-Contex branch from 958c9e3 to 4bf8ba1 Compare June 7, 2019 18:10

Fix build

0887b0d

draffensperger approved these changes Jun 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP Propagation: set `Correlation-Context` header #567

HTTP Propagation: set `Correlation-Context` header #567

mayurkale22 commented May 30, 2019

codecov-io commented May 30, 2019 •

edited

Loading

songy23 May 31, 2019

mayurkale22 May 31, 2019

draffensperger May 31, 2019

mayurkale22 May 31, 2019

draffensperger May 31, 2019

songy23 May 31, 2019

draffensperger May 31, 2019

mayurkale22 May 31, 2019

draffensperger May 31, 2019

draffensperger Jun 8, 2019

songy23 May 31, 2019

draffensperger Jun 8, 2019

HTTP Propagation: set Correlation-Context header #567

Are you sure you want to change the base?

HTTP Propagation: set Correlation-Context header #567

Conversation

mayurkale22 commented May 30, 2019

codecov-io commented May 30, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HTTP Propagation: set `Correlation-Context` header #567

HTTP Propagation: set `Correlation-Context` header #567

codecov-io commented May 30, 2019 •

edited

Loading