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.

Sign up for GitHub

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

Jump to bottom

feat: add jaeger http trace format (#696) #701

Merged

mayurkale22 merged 30 commits into open-telemetry:master from DotSpy:jaeger-format

Jan 20, 2020

Member

DotSpy commented Jan 16, 2020

Which problem is this PR solving?

Closes Add support for jaeger propagation formats #696

Short description of the changes

add jaeger http trace format for working with uber-trace-id header


          feat: add jaeger http trace format (open-telemetry#696)

d4de43e

DotSpy requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners

January 16, 2020 09:54

codecov-io commented Jan 16, 2020 •

edited

Codecov Report

❗ No coverage uploaded for pull request base (master@e0e7b85). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #701   +/-   ##
=========================================
  Coverage          ?   91.01%           
=========================================
  Files             ?      221           
  Lines             ?    10366           
  Branches          ?      959           
=========================================
  Hits              ?     9435           
  Misses            ?      931           
  Partials          ?        0

vladislav-kiva added 2 commits

January 16, 2020 13:14


          feat: add jaeger http trace format (open-telemetry#696)

ae93d80


          feat: add jaeger http trace format (open-telemetry#696)

4b2a9d3

dyladan reviewed

View reviewed changes

Member

dyladan left a comment

This is a great start, thanks for this.

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

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

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

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

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

vladislav-kiva added 2 commits

January 16, 2020 16:36


          feat: add jaeger http trace format (open-telemetry#696)

1a206cb


          feat: add jaeger http trace format (open-telemetry#696)

b7a345f

Member

dyladan commented Jan 16, 2020

You just need to fix the formatting now by running yarn run fix.

Contributor

vladislav-kiva commented Jan 16, 2020

yarn run fix

You just need to fix the formatting now by running yarn run fix.

sorry first time touching js ts and etc

dyladan approved these changes

View reviewed changes


          feat: add jaeger http trace format (open-telemetry#696)

ee56b19

dyladan reviewed

View reviewed changes

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


          fix: we should set sampled\unsampled via flag

0fa26ef

dyladan requested changes

View reviewed changes

Member

dyladan left a comment

See my comment previously but the mechanism you used will convert it to decimal not hex and only will give a single character.

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

vladislav-kiva added 2 commits

January 16, 2020 17:41


          fix: we should set sampled\unsampled via flag

b0958ee


          fix: flags should be converted to hex, not decimal

aee5f41

DotSpy requested a review from dyladan

January 16, 2020 16:02

dyladan approved these changes

View reviewed changes

OlivierAlbertini reviewed

View reviewed changes

Member

OlivierAlbertini left a comment •

edited

Should it be in a new package ? I don't believe that core should have something with Jaeger implementation. Because after that we could have Zipkin, Elastic APM etc...

OlivierAlbertini reviewed

View reviewed changes

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

Member

mayurkale22 commented Jan 16, 2020

Should it be in a new package ? I don't believe that core should have something with Jaeger implementation. Because after that we could have Zipkin, Elastic APM etc...

+1 I would suggest to create a new package (something like opentelemetry-propagation-jaeger).


          Merge branch 'master' into jaeger-format

e946b6a

DotSpy requested a review from OlivierAlbertini

January 17, 2020 08:10


          test: add test with span generated by jaeger client

5c3db80

Member Author

DotSpy commented Jan 17, 2020

Deployed it locally and it's working, i'm very happy :)

dyladan approved these changes

View reviewed changes

Member

dyladan left a comment

Think this looks good now. Thanks for doing this :)

obecny suggested changes

View reviewed changes

Member

obecny left a comment

lgtm, added few comments - some minor changes, but the urls in readme needs to be changed,
thx

packages/opentelemetry-propagator-jaeger/README.md Outdated

+              [![devDependencies][devDependencies-image]][devDependencies-url]
+              [![Apache License][license-image]][license-image]
+              OpenTelemetry propagation Jaeger provide HTTP header propagation for systems that using Jaeger HTTP header format.

Member

obecny Jan 17, 2020

What would you say for such description?:
OpenTelemetry Jaeger Propagator provides HTTP header propagation for systems that are using Jaeger HTTP header format.

packages/opentelemetry-propagator-jaeger/README.md Outdated

+              [license-url]: https://github.com/open-telemetry/opentelemetry-js/blob/master/LICENSE
+              [license-image]: https://img.shields.io/badge/license-Apache_2.0-green.svg?style=flat
+              [dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/status.svg?path=packages/opentelemetry-core
+              [dependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-core

Member

obecny Jan 17, 2020

you have wrong url, it should be opentelemetry-propagator-jaeger instead of opentelemetry-core

packages/opentelemetry-propagator-jaeger/README.md Outdated

+              [license-image]: https://img.shields.io/badge/license-Apache_2.0-green.svg?style=flat
+              [dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/status.svg?path=packages/opentelemetry-core
+              [dependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-core
+              [devDependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/dev-status.svg?path=packages/opentelemetry-core

Member

obecny Jan 17, 2020

you have wrong url, it should be opentelemetry-propagator-jaeger instead of opentelemetry-core

packages/opentelemetry-propagator-jaeger/README.md Outdated

+              [gitter-url]: https://gitter.im/open-telemetry/opentelemetry-node?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge
+              [license-url]: https://github.com/open-telemetry/opentelemetry-js/blob/master/LICENSE
+              [license-image]: https://img.shields.io/badge/license-Apache_2.0-green.svg?style=flat
+              [dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/status.svg?path=packages/opentelemetry-core

Member

obecny Jan 17, 2020

you have wrong url, it should be opentelemetry-propagator-jaeger instead of opentelemetry-core

packages/opentelemetry-propagator-jaeger/README.md Outdated

+              [dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/status.svg?path=packages/opentelemetry-core
+              [dependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-core
+              [devDependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/dev-status.svg?path=packages/opentelemetry-core
+              [devDependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-core&type=dev

Member

obecny Jan 17, 2020

you have wrong url, it should be opentelemetry-propagator-jaeger instead of opentelemetry-core

packages/opentelemetry-propagator-jaeger/README.md Outdated

+              [devDependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/dev-status.svg?path=packages/opentelemetry-core
+              [devDependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-core&type=dev
+              [npm-url]: https://www.npmjs.com/package/@opentelemetry/core
+              [npm-img]: https://badge.fury.io/js/%40opentelemetry%2Fcore.svg

Member

obecny Jan 17, 2020

you have wrong url, it should be propagator-jaeger instead of core

packages/opentelemetry-propagator-jaeger/package.json Outdated

+              {
+                "name": "@opentelemetry/propagator-jaeger",
+                "version": "0.3.2",
+                "description": "OpenTelemetry propagator Jaeger provide HTTP header propagation for systems that using Jaeger HTTP header format",

Member

obecny Jan 17, 2020

And the same here?:
OpenTelemetry Jaeger Propagator provides HTTP header propagation for systems that are using Jaeger HTTP header format.

packages/opentelemetry-propagator-jaeger/src/context/propagation/JaegerHttpTraceFormat.ts Outdated

+               * Propagates {@link SpanContext} through Trace Context format propagation.
+               * {trace-id}:{span-id}:{parent-span-id}:{flags}
+               * {trace-id}
+               * 64-bit or 128-bit random number in base16 format

Member

obecny Jan 17, 2020

comma in the end ?, and this should apply for other comments too

packages/opentelemetry-propagator-jaeger/src/context/propagation/JaegerHttpTraceFormat.ts Outdated

+                  this._jaegerTraceHeader = customTraceHeader || UBER_TRACE_ID_HEADER;
+                }
+                inject(

Member

obecny Jan 17, 2020

pls add some jsdoc

packages/opentelemetry-propagator-jaeger/src/context/propagation/JaegerHttpTraceFormat.ts Outdated

+                  ] = `${spanContext.traceId}:${spanContext.spanId}:0:${traceFlags}`;
+                }
+                extract(

Member

obecny Jan 17, 2020

pls add some jsdoc

Member

obecny commented Jan 17, 2020

@DotSpy could you please add the mentioned screenshot also to the readme, so that it is shown nicely when you view the package ?

mayurkale22 reviewed

View reviewed changes

packages/opentelemetry-propagator-jaeger/src/index.ts Outdated

+               * limitations under the License.
+               */
+              export * from './context/propagation/JaegerHttpTraceFormat.js';

Member

mayurkale22 Jan 17, 2020

Can we keep JaegerTraceFormat in src/ dirrectly, why do we need to have sub-folder?

Member

mayurkale22 Jan 17, 2020

Also, this should be just export * from './context/propagation/JaegerHttpTraceFormat'; or export * from './JaegerHttpTraceFormat';

Member Author

DotSpy Jan 17, 2020

@mayurkale22 i did it like in opentelemetry-core to keep the same structure. I don't know how better, because this is my first experience with this technology stack. In other languages like Java, Scala, Kotlin and etc. we keeping the same structure for the same responsibilities.

Member

dyladan Jan 17, 2020

In node it is very common to have a package with a single file in no folder structure of any kind if it only fulfills a single purpose.

DotSpy added 2 commits

January 17, 2020 23:09


          fix: apply review changes


          fix: move out from sub dirs

obecny approved these changes

View reviewed changes

Member

obecny left a comment •

edited

Thx for these changes.
One thing and some explanation for this description
"OpenTelemetry propagator-jaeger provides HTTP header propagation for systems that are using Jaeger HTTP header format"

propagator-jaeger is good name when we name package so the the namespace is correct.
For example plugin-xml-http-request, plugin-http, plugin-mysql etc. But when you are describing what it is you are using common language, that's why we have MySql Plugin and not Plugin MySql, XMLHttpRequest Plugin and not Plugin XMLHttpRequest. So the same applies here.
That's why I think it would sounds more naturally when you say Yaeger Propagator, instead Propagator Yaeger, That's why in description I think we should use this form Yaeger Propagator when we say about @opentelemetry/propagator-yaeger.
And if in future we want to have more propagators it will be @opentelemetry/propagator-xyz and we will call it XYZ Propagator.
I hope my explanation is fine :) besides that great job with your's Yaeger Propagator :)


          Merge branch 'master' into jaeger-format

8b072b3

Member Author

DotSpy commented Jan 17, 2020 •

edited

"OpenTelemetry propagator-jaeger provides HTTP header propagation for systems that are using Jaeger HTTP header format"

ohh thanks, got it, will change


          docs: use common language for docs

bda9831

mayurkale22 approved these changes

View reviewed changes

mayurkale22 added enhancement Merge:LGTM labels

Member Author

DotSpy commented Jan 17, 2020

For now i do not understand why it's failing, any ideas?

Member

dyladan commented Jan 17, 2020

For now i do not understand why it's failing, any ideas?

require.context only works when used with webpack. your node 8/10/12 tests are attempting to run the index-webpack.ts file as a regular test. Not sure why it is happening here and doesn't happen in the core tests. Maybe @obecny can shed some light.

Member

obecny commented Jan 17, 2020

For now i do not understand why it's failing, any ideas?

require.context only works when used with webpack. your node 8/10/12 tests are attempting to run the index-webpack.ts file as a regular test. Not sure why it is happening here and doesn't happen in the core tests. Maybe @obecny can shed some light.

try to replace in package.json the test script with this one
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts' --exclude 'test/index-webpack.ts'",

DotSpy added 2 commits

January 18, 2020 01:01


          fix: test script fix

e0e7b85


          Merge branch 'master' into jaeger-format

b5c900d

Member

dyladan commented Jan 17, 2020

For now i do not understand why it's failing, any ideas?

require.context only works when used with webpack. your node 8/10/12 tests are attempting to run the index-webpack.ts file as a regular test. Not sure why it is happening here and doesn't happen in the core tests. Maybe @obecny can shed some light.

try to replace in package.json the test script with this one
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts' --exclude 'test/index-webpack.ts'",

Why does core not do this? https://github.com/open-telemetry/opentelemetry-js/blob/021bbbbee9356ff56c4b703633e0f67d16b3e425/packages/opentelemetry-core/package.json

Member Author

DotSpy commented Jan 17, 2020

For now i do not understand why it's failing, any ideas?

require.context only works when used with webpack. your node 8/10/12 tests are attempting to run the index-webpack.ts file as a regular test. Not sure why it is happening here and doesn't happen in the core tests. Maybe @obecny can shed some light.

try to replace in package.json the test script with this one
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts' --exclude 'test/index-webpack.ts'",

Why does core not do this? https://github.com/open-telemetry/opentelemetry-js/blob/021bbbbee9356ff56c4b703633e0f67d16b3e425/packages/opentelemetry-core/package.json

i think this issue with test directories in opentelemetry-propagator-jaeger

vmarchaud approved these changes

View reviewed changes

Member

vmarchaud left a comment

LGTM

OlivierAlbertini approved these changes

View reviewed changes

OlivierAlbertini reviewed

View reviewed changes

packages/opentelemetry-propagator-jaeger/src/JaegerHttpTraceFormat.ts

+                 * @param {string} [customTraceHeader="uber-trace-id"] - HTTP header to inject\extract trace from.
+                 **/
+                constructor(customTraceHeader?: string) {
+                  this._jaegerTraceHeader = customTraceHeader || UBER_TRACE_ID_HEADER;

Member

OlivierAlbertini Jan 20, 2020

👍

mayurkale22 merged commit 6de9afb into open-telemetry:master

DotSpy mentioned this pull request

REQUEST: New membership for @DotSpy open-telemetry/community#304

Closed

6 tasks

DotSpy deleted the jaeger-format branch

January 15, 2021 10:51

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


          feat: add jaeger http trace format (open-telemetry#696) (open-telemet…

82091b1

…ry#701)

* feat: add jaeger http trace format (open-telemetry#696)

* feat: add jaeger http trace format (open-telemetry#696)

* feat: add jaeger http trace format (open-telemetry#696)

* feat: add jaeger http trace format (open-telemetry#696)

* feat: add jaeger http trace format (open-telemetry#696)

* feat: add jaeger http trace format (open-telemetry#696)

* fix: we should set sampled\unsampled via flag

* fix: we should set sampled\unsampled via flag

* fix: flags should be converted to hex, not decimal

* feat: create new package for propagation jaeger

* fix: remove unused dependencies, correct readme header, moved out jaeger from core index.ts

* fix: added jaeger keyword

* fix: remove comma

* docs: replace NodeTracer with NodeTracerRegistry

* fix: added missing jaeger keyword to exporter-jaeger

* fix: remove test for browser

* fix: remove yarn for browser

* fix: use same naming style as other packages

* feat: added index.ts and version.ts, revert test for browser

* fix: tests added index-webpack.ts

* test: add test with span generated by jaeger client

* fix: apply review changes

* fix: move out from sub dirs

* docs: use common language for docs

* fix: test script fix

Co-authored-by: Uladzislau Kiva <vladislav.kiva@moneyman.ru>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dyladan dyladan approved these changes

OlivierAlbertini OlivierAlbertini approved these changes

vladislav-kiva vladislav-kiva left review comments

obecny obecny approved these changes

vmarchaud vmarchaud approved these changes

mayurkale22 mayurkale22 approved these changes

bg451 Awaiting requested review from bg451

markwolff Awaiting requested review from markwolff

rochdev Awaiting requested review from rochdev