feat: implement W3C Correlation Context propagator #838

rubenvp8510 · 2020-03-03T21:36:20Z

Which problem is this PR solving?

Fixes Implement W3C Correlation Context propagator #833

Short description of the changes

This PR implements propagation for correlation context

codecov-io · 2020-03-03T21:54:59Z

Codecov Report

Merging #838 into master will increase coverage by 0.04%.
The diff coverage is 99.09%.

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
+ Coverage   95.12%   95.17%   +0.04%     
==========================================
  Files         213      216       +3     
  Lines        8952     9063     +111     
  Branches      806      819      +13     
==========================================
+ Hits         8516     8626     +110     
- Misses        436      437       +1

Impacted Files	Coverage Δ
...tion-context/propagation/HttpCorrelationContext.ts	`98.00% <98.00%> (ø)`
...ore/src/correlation-context/correlation-context.ts	`100.00% <100.00%> (ø)`
...correlation-context/HttpCorrelationContext.test.ts	`100.00% <100.00%> (ø)`

packages/opentelemetry-core/src/distribute-context/propagation/HttpTraceContext.ts

mayurkale22 · 2020-03-03T22:28:48Z

packages/opentelemetry-core/src/distribute-context/propagation/HttpTraceContext.ts

+      const headerValue = Object.keys(distContext)
+        .map(
+          (key: string) =>
+            `${encodeURIComponent(key)}=${encodeURIComponent(


I think we should consider below limits as per the specs: https://w3c.github.io/correlation-context/#header-value

Limits:

Maximum number of name-value pairs: 180.

Maximum number of bytes per a single name-value pair: 4096.

Maximum total length of all name-value pairs: 8192.

+1, I'll d those limits.

Should we throw an exception when the limits are exceeded? or skip the key-value pair? WDYT?

In general we never throw if we can avoid it. I would suggest to log and skip WDYT @mayurkale22

This may be a question for spec as well.

packages/opentelemetry-core/src/distribute-context/propagation/HttpTraceContext.ts

packages/opentelemetry-core/src/distribute-context/distribute-context.ts

packages/opentelemetry-core/src/distribute-context/propagation/HttpTraceContext.ts

rubenvp8510 · 2020-03-26T04:49:50Z

Addressed all observations

rubenvp8510 · 2020-04-13T20:48:29Z

Any other observation on this ?

dyladan · 2020-04-13T22:13:57Z

I am waiting for the spec issue to be resolved before merging this. The spec header name has had some back and forth.

dyladan · 2020-05-15T12:13:58Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+  setCorrelationContext,
+} from '../correlation-context';
+
+export const CORRELATION_CONTEXT_HEADER = 'otcorrelationcontext';


This header should be otcorrelations per the spec PR which was merged today. After this change this PR is mergeable. Sorry this took so long.

Changed

Thanks for the review.

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

mayurkale22 · 2020-05-15T18:03:17Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+  inject(context: Context, carrier: unknown, setter: SetterFunction) {
+    const distContext = getCorrelationContext(context);
+    if (distContext) {
+      const all = Object.keys(distContext);


s/all/keys?

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

packages/opentelemetry-core/test/correlation-context/HttpCorrelationContext.test.ts

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

Signed-off-by: Ruben Vargas <[email protected]> Signed-off-by: Ruben Vargas <[email protected]>

Signed-off-by: Ruben Vargas <[email protected]>

obecny · 2020-05-18T11:31:41Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+    const correlationContext = getCorrelationContext(context);
+    if (!correlationContext) return;
+    const all = Object.keys(correlationContext);
+    const values = all


Please make an util helper function from this, and the same for values.reduce, it is hard to understand what is happening here, without looking and analysing the code

obecny · 2020-05-18T11:34:17Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+    const pairs = headerValue.split(ITEMS_SEPARATOR);
+    if (pairs.length == 1) return context;
+    pairs.forEach(entry => {
+      const valueProps = entry.split(PROPERTIES_SEPARATOR);


consider creating a helper function, maybe a parseHeader or parsePair etc. ?

obecny · 2020-05-18T11:35:48Z

packages/opentelemetry-core/test/correlation-context/HttpCorrelationContext.test.ts

+    });
+  });
+
+  it('returns null if header is missing', () => {


it says null, but is being compared against undefined

obecny · 2020-05-18T11:42:21Z

packages/opentelemetry-core/test/correlation-context/HttpCorrelationContext.test.ts

+
+      let value = '';
+      // Generate long value 2*MAX_PER_NAME_VALUE_PAIRS
+      for (let i = 0; i < MAX_PER_NAME_VALUE_PAIRS; ++i) {


I think it is already safe to use repeat here

value = '1a'.repeat(MAX_PER_NAME_VALUE_PAIRS_);

obecny · 2020-05-18T11:45:10Z

packages/opentelemetry-core/src/correlation-context/correlation-context.ts

+  'OpenTelemetry Distributed Contexts Key'
+);
+
+export function getCorrelationContext(


missing tsdoc

obecny · 2020-05-18T11:45:18Z

packages/opentelemetry-core/src/correlation-context/correlation-context.ts

+  );
+}
+
+export function setCorrelationContext(


missing tsdoc

obecny · 2020-05-18T11:45:24Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+
+/* W3C Constrains*/
+
+export const MAX_NAME_VALUE_PAIRS = 180;


missing tsdoc

obecny · 2020-05-18T11:45:28Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+/* W3C Constrains*/
+
+export const MAX_NAME_VALUE_PAIRS = 180;
+export const MAX_PER_NAME_VALUE_PAIRS = 4096;


missing tsdoc

obecny · 2020-05-18T11:45:37Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+
+export const MAX_NAME_VALUE_PAIRS = 180;
+export const MAX_PER_NAME_VALUE_PAIRS = 4096;
+export const MAX_TOTAL_LENGTH = 8192;


missing tsdoc

obecny · 2020-05-18T12:01:41Z

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

+  setCorrelationContext,
+} from '../correlation-context';
+
+export const CORRELATION_CONTEXT_HEADER = 'otcorrelations';


missing tsdoc

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts

mayurkale22

Added one comment, overall LGTM.

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2020-05-18T19:25:20Z

Done with all comments.

obecny

lgtm, thx for those changes

mayurkale22 · 2020-05-18T20:50:55Z

It would be nice if you can add this into Built-in Propagators (in a separate PR) doc (See: https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-core#built-in-propagators).

rubenvp8510 requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners March 3, 2020 21:36

rubenvp8510 force-pushed the dist-context-propagation branch 3 times, most recently from e82d1c9 to 768fd05 Compare March 3, 2020 21:43

dyladan reviewed Mar 3, 2020

View reviewed changes

packages/opentelemetry-core/src/distribute-context/propagation/HttpTraceContext.ts Outdated Show resolved Hide resolved

mayurkale22 reviewed Mar 3, 2020

View reviewed changes

Flarna reviewed Mar 4, 2020

View reviewed changes

packages/opentelemetry-core/src/distribute-context/propagation/HttpTraceContext.ts Outdated Show resolved Hide resolved

dyladan mentioned this pull request Mar 4, 2020

Beta Readiness Tracker #782

Closed

46 tasks

rubenvp8510 force-pushed the dist-context-propagation branch 2 times, most recently from 61d6eba to 8b570dc Compare March 26, 2020 04:49

rubenvp8510 requested review from dyladan and mayurkale22 April 1, 2020 00:31

dyladan reviewed May 15, 2020

View reviewed changes

rubenvp8510 force-pushed the dist-context-propagation branch 3 times, most recently from 27e47fd to 931ffaa Compare May 15, 2020 15:50

dyladan approved these changes May 15, 2020

View reviewed changes

mayurkale22 reviewed May 15, 2020

View reviewed changes

mayurkale22 added the enhancement New feature or request label May 15, 2020

mayurkale22 added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 15, 2020

rubenvp8510 force-pushed the dist-context-propagation branch 2 times, most recently from 9839316 to 0ff1c15 Compare May 18, 2020 02:25

rubenvp8510 added 4 commits May 17, 2020 22:41

feat: implement W3C Correlation Context propagator

78758d3

Signed-off-by: Ruben Vargas <[email protected]> Signed-off-by: Ruben Vargas <[email protected]>

chore: change correlation context header name according to the spec

923dddb

Signed-off-by: Ruben Vargas <[email protected]>

chore: add tests with spaces, fix style

5d93ffb

Signed-off-by: Ruben Vargas <[email protected]>

chore: remove nested conditions on correlation context extract

193b5f9

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the dist-context-propagation branch from ac11b40 to 193b5f9 Compare May 18, 2020 03:41

vmarchaud approved these changes May 18, 2020

View reviewed changes

obecny reviewed May 18, 2020

View reviewed changes

mayurkale22 reviewed May 18, 2020

View reviewed changes

packages/opentelemetry-core/src/correlation-context/propagation/HttpCorrelationContext.ts Show resolved Hide resolved

mayurkale22 approved these changes May 18, 2020

View reviewed changes

chore: tsdocs, separate some functions, add export to index.ts

4f7adae

Signed-off-by: Ruben Vargas <[email protected]>

mayurkale22 approved these changes May 18, 2020

View reviewed changes

mayurkale22 requested a review from obecny May 18, 2020 19:28

obecny approved these changes May 18, 2020

View reviewed changes

dyladan approved these changes May 18, 2020

View reviewed changes

Merge branch 'master' into dist-context-propagation

14d7448

mayurkale22 merged commit f978377 into open-telemetry:master May 19, 2020

mayurkale22 mentioned this pull request May 19, 2020

Document CorrelationContext propagator under Built-in Implementations section #1079

Closed

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

docs: document node v10 EOL & v16 supported (open-telemetry#838)

dbc9a33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement W3C Correlation Context propagator #838

feat: implement W3C Correlation Context propagator #838

rubenvp8510 commented Mar 3, 2020

codecov-io commented Mar 3, 2020 •

edited

Loading

mayurkale22 Mar 3, 2020

rubenvp8510 Mar 3, 2020

rubenvp8510 Mar 25, 2020 •

edited

Loading

dyladan Mar 25, 2020

rubenvp8510 commented Mar 26, 2020

rubenvp8510 commented Apr 13, 2020

dyladan commented Apr 13, 2020

dyladan May 15, 2020

rubenvp8510 May 15, 2020

mayurkale22 May 15, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

obecny May 18, 2020

mayurkale22 left a comment

rubenvp8510 commented May 18, 2020

obecny left a comment

mayurkale22 commented May 18, 2020

feat: implement W3C Correlation Context propagator #838

feat: implement W3C Correlation Context propagator #838

Conversation

rubenvp8510 commented Mar 3, 2020

Which problem is this PR solving?

Short description of the changes

codecov-io commented Mar 3, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 commented Mar 26, 2020

rubenvp8510 commented Apr 13, 2020

dyladan commented Apr 13, 2020

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

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

rubenvp8510 commented May 18, 2020

obecny left a comment

Choose a reason for hiding this comment

mayurkale22 commented May 18, 2020

codecov-io commented Mar 3, 2020 •

edited

Loading

rubenvp8510 Mar 25, 2020 •

edited

Loading