feat: add AWS Xray Propagator #163

EdZou · 2020-07-26T22:07:33Z

Which problem is this PR solving?

As discussed on Node SIG meeting, adding AWS Xray propagator components under opentelemetry-js-contrib/propagators repository

Short description of the changes

Add AWS Xray propagator components
Add corresponding unit test for AWS Xray propagator
Add corresponding README.md file for AWS Xray propagator

codecov · 2020-07-26T22:11:15Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   94.10%   94.33%   +0.22%     
==========================================
  Files          74       78       +4     
  Lines        3582     3762     +180     
  Branches      387      409      +22     
==========================================
+ Hits         3371     3549     +178     
- Misses        211      213       +2

Impacted Files	Coverage Δ
...ors/opentelemetry-propagator-aws-xray/.eslintrc.js	`0.00% <0.00%> (ø)`
...s/opentelemetry-propagator-aws-xray/src/version.ts	`0.00% <0.00%> (ø)`
...metry-propagator-aws-xray/src/AWSXRayPropagator.ts	`100.00% <0.00%> (ø)`
...propagator-aws-xray/test/AWSXRayPropagator.test.ts	`100.00% <0.00%> (ø)`

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

anuraaga · 2020-07-27T03:11:58Z

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

+    let parsedTraceId = INVALID_TRACE_ID;
+    let parsedSpanId = INVALID_SPAN_ID;
+    let parsedTraceFlags = null;
+    while(pos < traceHeader.length) {


In Java, this is quite a bit faster than using split, but I don't know about JS, it may be better to stick to using split here.

I am also confused here, I initially tried to use split method but the Java implementation is in this way so I just followed it

There is a benchmark utility you can use to test things like this. You can see an example at open-telemetry/opentelemetry-js#1334

You can also use jsperf.com for browser benchmarking.

Subjectively, I think split based implementations are typically easier to read, which is an important distinction. I would expect your implementation here to be slightly faster than a split implementation, but not appreciably.

If the ids are fixed length, it might be easier and faster to use substring also.

I now think Daniel's consideration makes more sense. Since we have a fixed size tracing header, it can be a little bit faster to use substring, and maybe that is why Java also implements in this way

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

dyladan · 2020-07-27T13:13:54Z

propagators/opentelemetry-propagator-aws-xray/package.json

@@ -0,0 +1,76 @@
+{
+  "name": "opentelemetry-propagator-aws-xray",


Suggested change

"name": "opentelemetry-propagator-aws-xray",

"name": "@opentelemetry/propagator-aws-xray",

@open-telemetry/javascript-maintainers do we want to have another namespace or even just a naming convention for vendor-specific and contrib things? Maybe @otel-contrib/propagator-aws-xray or @opentelemetry/contrib-propagator-aws-xray?

Name still needs to be changed

No problem, which one should it be changed to? @opentelemetry/contrib-propagator-aws-xray or @otel-contrib/propagator-aws-xray?

For now, lets do @opentelemetry/propagator-aws-xray. I will bring up the naming discussion at tomorrow's SIG meeting.

Got it! Thank you so much!

dyladan · 2020-07-27T13:21:04Z

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

+    let parsedTraceId = INVALID_TRACE_ID;
+    let parsedSpanId = INVALID_SPAN_ID;
+    let parsedTraceFlags = null;
+    while(pos < traceHeader.length) {


There is a benchmark utility you can use to test things like this. You can see an example at open-telemetry/opentelemetry-js#1334

You can also use jsperf.com for browser benchmarking.

dyladan · 2020-07-27T13:23:25Z

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

+    let parsedTraceId = INVALID_TRACE_ID;
+    let parsedSpanId = INVALID_SPAN_ID;
+    let parsedTraceFlags = null;
+    while(pos < traceHeader.length) {


Subjectively, I think split based implementations are typically easier to read, which is an important distinction. I would expect your implementation here to be slightly faster than a split implementation, but not appreciably.

dyladan · 2020-07-27T13:24:09Z

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

+    let parsedTraceId = INVALID_TRACE_ID;
+    let parsedSpanId = INVALID_SPAN_ID;
+    let parsedTraceFlags = null;
+    while(pos < traceHeader.length) {


If the ids are fixed length, it might be easier and faster to use substring also.

dyladan · 2020-07-27T13:24:40Z

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

+  private _parseTraceFlag(xraySampledFlag: string): TraceFlags | null {
+    if (xraySampledFlag.length === SAMPLED_FLAG_LENGTH && xraySampledFlag === NOT_SAMPLED) {
+      return TraceFlags.NONE;
+    } else if (xraySampledFlag === IS_SAMPLED) {


No need for else after a return

propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts

EdZou · 2020-07-30T01:09:00Z

Hello, Daniel @dyladan
After discussion with my mentor Anuraag @anuraaga , we believe according to the specification here: https://github.com/open-telemetry/opentelemetry-specification/pull/735/files. We should add our AWS Xray propagator code under js-contrib repo anyway

dyladan · 2020-07-30T15:01:32Z

I have discussed this with the other maintainers @obecny and @mayurkale22 and we would like to keep vendor-specific code out of our hosted repositories if at all possible. We are happy to code-review vendor propagators and exporters, but we would prefer that these code modules be hosted and deployed from vendor-owned repositories. As an example, you can look at what GCP has done for their cloud trace and cloud monitoring exporters and propagator here https://github.com/GoogleCloudPlatform/opentelemetry-operations-js. At the time this repository was created, we had similar discussions with the GCP folks.

anuraaga · 2020-07-30T15:24:32Z

@dyladan Do you mind adding your perspective to the linked spec PR?

https://github.com/open-telemetry/opentelemetry-specification/pull/735/files

It'd be better to do that before it gets merged - while yes it only says SHOULD, I think that's usually considered a recommendation since otherwise there's no reason to mention it at all. Not going with the recommendation is something that needs to have some real motivation I think.

AWS can host the module - if that's the final decision we're happy to. But I see discrepancy between JS, Java, and the spec in this area where there doesn't seem to be a huge reason to have that discrepancy. Propagators being an interop concern, therefore part of the SDKs does make sense to me, but they do feel vendorish and wanting to split makes sense to me too. My personal opinion is wanting consistency there between Java and JS one way or another though (those are the two languages I've looked at in depth so far).

anuraaga · 2020-08-24T01:45:47Z

@EdZou npm link is an alternative to publishing, it should generally work. When we publish that package it will be a different name and we'll have an aws namespace for it like @aws-observability/opentelemetry-propagator-xray.

Anyways, I believe lerna link should work well enough for your example project, but I notice now that at least in your PR the example doesn't have dependencies on the xray packages?

https://github.com/open-o11y/aws-opentelemetry-js/pull/3/files#diff-640d1aab881301a5fabb1482b0fa1d21R24

EdZou · 2020-08-24T01:51:29Z

Yes, I did not explicitly add dependency in package.json, but it still works for instrumenting IdGenerator. Not so sure if that is the key probelm here @anuraaga

anuraaga · 2020-08-24T06:58:16Z

@EdZou @dyladan I debugged the example app and while I didn't confirm for sure, I guess we're waiting on this fix to be released open-telemetry/opentelemetry-js#1387 @dyladan do you think it's possible to make a release so propagators in external packages can work ok?

feat: add AWS Xray Propagator

53f987a

fix: add eslint confiuration file

1eb16f1

anuraaga reviewed Jul 27, 2020

View reviewed changes

dyladan reviewed Jul 27, 2020

View reviewed changes

fix: delete overkilling condition and fix lint errors

f9e0813

EdZou marked this pull request as ready for review July 28, 2020 01:12

EdZou requested a review from a team July 28, 2020 01:12

chore: modify package name

ad8ef96

dyladan mentioned this pull request Jul 30, 2020

Specify the initial propagators requirements. open-telemetry/opentelemetry-specification#735

Merged

This comment has been minimized.

Sign in to view

EdZou closed this Aug 26, 2020

obecny added the enhancement New feature or request label Sep 7, 2020

anuraaga mentioned this pull request Apr 13, 2021

feat: add xray propagator #421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add AWS Xray Propagator #163

feat: add AWS Xray Propagator #163

EdZou commented Jul 26, 2020

codecov bot commented Jul 26, 2020 •

edited

Loading

anuraaga Jul 27, 2020

EdZou Jul 27, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

EdZou Jul 27, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

EdZou Jul 28, 2020 •

edited

Loading

dyladan Jul 28, 2020 •

edited

Loading

EdZou Jul 28, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

dyladan Jul 27, 2020

EdZou commented Jul 30, 2020

dyladan commented Jul 30, 2020

anuraaga commented Jul 30, 2020

This comment has been minimized.

anuraaga commented Aug 24, 2020

EdZou commented Aug 24, 2020

anuraaga commented Aug 24, 2020

		@@ -0,0 +1,76 @@
		{
		"name": "opentelemetry-propagator-aws-xray",

	"name": "opentelemetry-propagator-aws-xray",
	"name": "@opentelemetry/propagator-aws-xray",

feat: add AWS Xray Propagator #163

feat: add AWS Xray Propagator #163

Conversation

EdZou commented Jul 26, 2020

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 26, 2020 • 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

EdZou Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

dyladan Jul 28, 2020 • edited Loading

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

EdZou commented Jul 30, 2020

dyladan commented Jul 30, 2020

anuraaga commented Jul 30, 2020

This comment has been minimized.

anuraaga commented Aug 24, 2020

EdZou commented Aug 24, 2020

anuraaga commented Aug 24, 2020

codecov bot commented Jul 26, 2020 •

edited

Loading

EdZou Jul 28, 2020 •

edited

Loading

dyladan Jul 28, 2020 •

edited

Loading