Feat: Make ID generator configurable #1331

EdZou · 2020-07-21T03:20:38Z

Which problem is this PR solving?

The existing implementation of id generating only contains two functions while in the Java and Go repository, we provide an interface and default IdGenerator implementation.

Short description of the changes

added id generator interface under opentelemetry-core repo
migrated original id generator functions and corresponding unit tests by implementing IdGenerator interface.
modified TracerConfig and Tracer to make it configurable for users.

packages/opentelemetry-api/src/platform/node/IdGenerator.ts

packages/opentelemetry-core/src/platform/node/RandomIdGenerator.ts

packages/opentelemetry-core/src/platform/node/AWSXrayIdGenerator.ts

packages/opentelemetry-api/src/platform/node/index.ts

packages/opentelemetry-core/test/platform/AWSXrayIdGenerator.test.ts

anuraaga · 2020-07-21T03:33:46Z

packages/opentelemetry-core/test/platform/RandomIdGenerator.test.ts

+const IdGenerator = new RandomIdGenerator();
+const IdGeneratorWithParameters = new RandomIdGenerator(8, 16);
+
+describe('IdGeneratorConstructor', () => {


Once you remove the length parameters, make sure this is mostly just copy-paste from previous tests

I actually added beforeEach() keyword to separate obtaining value part and testing value part. Is that good or not?

packages/opentelemetry-tracing/src/types.ts

packages/opentelemetry-api/src/platform/node/IdGenerator.ts

dyladan

Hello and thanks for your first contribution.

I can see from the PR here that you are a little confused about what is meant by platform. In our case, AWS is not a platform, but node and web are. The platform folders allow us to compile the project for node and web in cases where the implementation must differ (for instance, Buffer is not supported in web so other mechanisms must be used). The ID Generator interface would not go into a platform-specific folder because the interface is platform agnostic. If the implementation differs between web and node, as our current id generation does, then the concrete implementations would go into platform folders.

I would suggest you move the interface into some location that is outside of the platform folder, and the concrete generator implementation should have a node and web version.

In addition, we typically keep vendor-specific code out of our repositories. For instance, you can see the google cloud trace and cloud monitoring exporters are kept in google-controlled repositories.

There are also some small stylistic issues which @anuraaga alluded to WRT method name casing and things like that.

I am happy to carve out some time if you would like to discuss these issues.

packages/opentelemetry-api/src/platform/node/IdGenerator.ts

packages/opentelemetry-core/test/platform/AWSXrayIdGenerator.test.ts

dyladan · 2020-07-21T13:56:56Z

packages/opentelemetry-core/src/platform/node/AWSXrayIdGenerator.ts

@@ -0,0 +1,46 @@
+/*


This definitely does not belong in core. I'm not sure it belongs in contrib either though. We have been keeping proprietary vendor code out of our repositories across all of otel AFAIK. Dis something change?

packages/opentelemetry-core/src/platform/node/index.ts

… generators

codecov · 2020-07-21T17:06:20Z

Codecov Report

Merging #1331 into master will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1331      +/-   ##
==========================================
- Coverage   93.73%   93.61%   -0.12%     
==========================================
  Files         144      149       +5     
  Lines        3991     4261     +270     
  Branches      808      870      +62     
==========================================
+ Hits         3741     3989     +248     
- Misses        250      272      +22

Impacted Files	Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts	`100.00% <100.00%> (ø)`
packages/opentelemetry-tracing/src/Tracer.ts	`100.00% <100.00%> (ø)`
...ages/opentelemetry-exporter-collector/src/types.ts	`100.00% <0.00%> (ø)`
.../opentelemetry-exporter-collector/src/transform.ts	`96.70% <0.00%> (ø)`
...lemetry-exporter-collector/src/transformMetrics.ts	`80.68% <0.00%> (ø)`
...kages/opentelemetry-exporter-collector/src/util.ts	`100.00% <0.00%> (ø)`
...ry-exporter-collector/src/CollectorExporterBase.ts	`94.73% <0.00%> (ø)`

obecny

Few things.

Move all interfaces to types.ts
You should mimic the same behaviour for browser and for node
When bundle is being compiled the compiler checks for "browser" field in package.json if you check it you will see that file ./src/platform/index.ts is replaced with file ./src/platform/browser/index.ts
When you write tests you should write a test that is not platform dependent, it means you add this in folder test. When the tests will be run in node it will pickup the id from node and run tests using this implementation. When tests will be run in karma it will use the browser from package.json replace the appropriate files and will run the test in browser environment. It means you write one test that should pass in both environments.
You are lacking here the whole implementation for browser

Besides all I have open questions

Is this required by spec ?
What are you trying to achieve ? Is this to be able to inject your own id generators ?

Concerns:

The AWSXrayIdGenerator should not be a part of api, this can be either in core package (it might make sense) or in contrib repo

…s web version

EdZou · 2020-07-22T18:05:41Z

Few things.

1. Move all interfaces to types.ts

2. You should mimic the same behaviour for browser and for node

3. When bundle is being compiled the compiler checks for "browser" field in package.json if you check it you will see that file `./src/platform/index.ts` is replaced with file `./src/platform/browser/index.ts`

4. When you write tests you should write a test that is not platform dependent, it means you add this in folder `test`. When the tests will be run in node it will pickup the `id` from node and run tests using this implementation. When tests will be run in karma it will use the `browser` from package.json replace the appropriate files and will run the test in browser environment. It means you write one test that should pass in both environments.

5. You are lacking here the whole implementation for browser

Besides all I have open questions

1. Is this required by spec ?

2. What are you trying to achieve ? Is this to be able to inject your own id generators ?

Concerns:

1. The AWSXrayIdGenerator should not be a part of api, this can be either in core package (it might make sense) or in contrib repo

Thank you for your comment! I have already added web version of IdGenerator
For your question:

I am not sure whether it is required by spec. But in both Java and Go repo they provide an interface for IdGenerator
Since we are going to implement AWS Xray IdGenerator, after discussion with my mentor, Anuraag, we believe provide an interface for not only AWS Xray IdGenerator will be good. Then I first add a interface and wrap original id generator function in a class.
Concerns:
Yes, I have already removed AWS part from this repo.
Again, thank you for your help!

packages/opentelemetry-core/src/platform/browser/RandomIdGenerator.ts

packages/opentelemetry-core/src/platform/node/RandomIdGenerator.ts

packages/opentelemetry-tracing/src/Tracer.ts

packages/opentelemetry-tracing/src/types.ts

dyladan · 2020-07-22T18:50:52Z

This looks a lot better after the changes, thanks

EdZou · 2020-07-22T19:16:51Z

Really thank you for reviewing my code and answering my questions! @dyladan @obecny @anuraaga
I believe current PR is ready for review and maybe being merged

packages/opentelemetry-core/src/platform/browser/RandomIdGenerator.ts

dyladan

LGTM. Thanks for you patience with the review process.

obecny

lgtm

anuraaga

Thanks!

packages/opentelemetry-tracing/src/types.ts

EdZou · 2020-07-30T01:04:30Z

It seems we still need 2 more maintainer's review to get this PR merged, what should we do now? @dyladan

dyladan · 2020-07-30T12:33:05Z

@EdZou you don't need maintainers, any approvers will do. @open-telemetry/javascript-approvers please take a look at this

dyladan · 2020-07-30T15:48:16Z

@EdZou I already pinged that group 3 hours ago. Please be patient as many of those people are contributing in their own free time, or have limited time their work allows them to dedicate to open source efforts.

EdZou · 2020-07-30T15:50:55Z

OMG, really sorry, I wrongly comprehend to let me take a look at the member of JS approver lists. Really sorry for that.

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* feat: graceful shutdown for tracing and metrics * fix: wording in test case * fix: typo * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: add callbacks to shutdown methods * fix: merge conflict * simplify meter shutdown code Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: fix one-liner * private function name style fix Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: naming of private member variables * fix: graceful shutdown now works in browser * fix: window event listener will trigger once * fix: modify global shutdown helper functions * fix: remove callback from remove listener args * fix: change global shutdown function names and simplify functionality * fix: add rest of function refactoring and simplification * fix: remove unintended code snippet * fix: refactor naming of listener cleanup function and fix sandbox issue * fix: make global shutdown cleanup local * fix: change interval of MeterProvider collection to ensure it does not trigger through clock * chore: removing _cleanupGlobalShutdownListeners * fix: remove unnecesary trace provider member function * Removing default span attributes (#1342) * refactor(opentelemetry-tracing): removing default span attributes Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * refactor(opentelemetry-tracing): removing default span attributed from tracer object Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * refactor(opentelemetry-tracing): removing accidental add to package.json Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * refactor(opentelemetry-tracing): removing redundant test and fixing suggestions by Shawn and Daniel Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * feat: add baggage support to the opentracing shim (#918) Co-authored-by: Mayur Kale <mayurkale@google.com> * Add nodejs sdk package (#1187) Co-authored-by: Naseem <naseemkullah@gmail.com> Co-authored-by: legendecas <legendecas@gmail.com> Co-authored-by: Mark Wolff <mrw00010@gmail.com> Co-authored-by: Matthew Wear <matthew.wear@gmail.com> * feat: add OTEL_LOG_LEVEL env var (#974) * Proto update to latest to support arrays and maps (#1339) * chore: 0.10.0 release proposal (#1345) * fix: add missing grpc-js index (#1358) * chore: 0.10.1 release proposal (#1359) * feat(api/context-base): change compile target to es5 (#1368) * Feat: Make ID generator configurable (#1331) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: require grpc-js instead of grpc in grpc-js example (#1364) Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com> * chore(deps): update all non-major dependencies (#1371) * chore: bump metapackage dependencies (#1383) * chore: 0.10.2 proposal (#1382) * fix: remove unnecesary trace provider member function * refactor(metrics): distinguish different aggregator types (#1325) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * Propagate b3 parentspanid and debug flag (#1346) * feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary (#1320) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * feat: Collector Metric Exporter for the Web (#1308) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * Fix issues in TypeScript getting started example code (#1374) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * chore: deploy canary releases (#1384) * fix: protos pull * fix: address marius' feedback * chore: deleting removeAllListeners from prometheus, fixing tests, cleanu of events when using shutdown notifier * fix: add documentation and cleanup code * fix: remove async label from shutdown and cleanup test case * fix: update controller collect to return promise * fix: make downsides of disabling graceful shutdown more apparent Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com> Co-authored-by: Aravin <34178459+aravinsiva@users.noreply.github.com> Co-authored-by: Ruben Vargas Palma <ruben.vp8510@gmail.com> Co-authored-by: Mayur Kale <mayurkale@google.com> Co-authored-by: Naseem <naseemkullah@gmail.com> Co-authored-by: legendecas <legendecas@gmail.com> Co-authored-by: Mark Wolff <mrw00010@gmail.com> Co-authored-by: Matthew Wear <matthew.wear@gmail.com> Co-authored-by: Naseem <naseem@transit.app> Co-authored-by: Mark Wolff <mark.wolff@microsoft.com> Co-authored-by: Cong Zou <32532612+EdZou@users.noreply.github.com> Co-authored-by: Reginald McDonald <40721169+reggiemcdonald@users.noreply.github.com> Co-authored-by: WhiteSource Renovate <bot@renovateapp.com> Co-authored-by: srjames90 <srjames@lightstep.com> Co-authored-by: David W <dwitt12345@gmail.com> Co-authored-by: Mick Dekkers <mickdekkersnl@gmail.com>

EdZou added 4 commits July 20, 2020 17:20

style(context): changed id generator from function to interface

5c1f4d5

fix: fix bugs of migrating id generators

89c24bb

feat: add AWS Xray IdGenerator and corresponding unit test

9443beb

feat: add configuration id generator part for tracer

e069a6d

anuraaga reviewed Jul 21, 2020

View reviewed changes

dyladan requested changes Jul 21, 2020

View reviewed changes

EdZou added 3 commits July 21, 2020 11:36

fix: modified annotations and method naming,deleted constructor of id…

0adcfc8

… generators

chore: added AWS Xray annotation

32a652d

chore: modified variables to start with lowercase

b4be35b

EdZou mentioned this pull request Jul 22, 2020

Hosting vendor supported exporters for OpenTelemetry #1335

Closed

obecny reviewed Jul 22, 2020

View reviewed changes

EdZou added 2 commits July 22, 2020 12:34

Merge remote-tracking branch 'upstream/master'

51fe01b

feat: remove AWS components, move interface of IdGenerator and add it…

6f31180

…s web version

EdZou changed the title ~~Feat: Migrated id generator to interface and Added AWS Xray IdGenerator~~ Feat: Migrated id generator to interface and Added configurable part for Tracer Jul 22, 2020

dyladan reviewed Jul 22, 2020

View reviewed changes

fix: fix indent problem and make Tracer._idGenerator private

6f51f62

EdZou marked this pull request as ready for review July 22, 2020 19:16

EdZou requested review from legendecas, markwolff, mayurkale22, mwear, naseemkullah, OlivierAlbertini and vmarchaud as code owners July 22, 2020 19:16

dyladan reviewed Jul 22, 2020

View reviewed changes

packages/opentelemetry-core/src/platform/browser/RandomIdGenerator.ts Outdated Show resolved Hide resolved

chore: remove declare word from web version IdGenerator

4703081

Merge branch 'master' into master

12b1940

dyladan approved these changes Jul 23, 2020

View reviewed changes

obecny approved these changes Jul 23, 2020

View reviewed changes

anuraaga approved these changes Jul 24, 2020

View reviewed changes

packages/opentelemetry-tracing/src/types.ts Outdated Show resolved Hide resolved

Merge branch 'master' into master

54e56c0

Update packages/opentelemetry-tracing/src/types.ts

b8374db

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

legendecas approved these changes Jul 31, 2020

View reviewed changes

vmarchaud approved these changes Jul 31, 2020

View reviewed changes

dyladan added the enhancement New feature or request label Jul 31, 2020

Merge branch 'master' into master

507fa48

dyladan changed the title ~~Feat: Migrated id generator to interface and Added configurable part for Tracer~~ Feat: Make ID generator configurable Jul 31, 2020

dyladan merged commit b7d6e74 into open-telemetry:master Jul 31, 2020

jonahrosenblum pushed a commit to jonahrosenblum/opentelemetry-js that referenced this pull request Aug 8, 2020

Feat: Make ID generator configurable (open-telemetry#1331)

802c5ad

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

NathanielRN mentioned this pull request Dec 15, 2020

Require the SDK to provide custom ID generation open-telemetry/opentelemetry-specification#1006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Make ID generator configurable #1331

Feat: Make ID generator configurable #1331

EdZou commented Jul 21, 2020 •

edited

Loading

anuraaga Jul 21, 2020

EdZou Jul 21, 2020

dyladan left a comment

dyladan Jul 21, 2020

codecov bot commented Jul 21, 2020 •

edited

Loading

obecny left a comment •

edited

Loading

EdZou commented Jul 22, 2020 •

edited

Loading

dyladan commented Jul 22, 2020

EdZou commented Jul 22, 2020 •

edited

Loading

dyladan left a comment

obecny left a comment

anuraaga left a comment

EdZou commented Jul 30, 2020

dyladan commented Jul 30, 2020

dyladan commented Jul 30, 2020

EdZou commented Jul 30, 2020 •

edited

Loading

Feat: Make ID generator configurable #1331

Feat: Make ID generator configurable #1331

Conversation

EdZou commented Jul 21, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

anuraaga Jul 21, 2020

Choose a reason for hiding this comment

EdZou Jul 21, 2020

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

dyladan Jul 21, 2020

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

obecny left a comment • edited Loading

Choose a reason for hiding this comment

EdZou commented Jul 22, 2020 • edited Loading

dyladan commented Jul 22, 2020

EdZou commented Jul 22, 2020 • edited Loading

dyladan left a comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

EdZou commented Jul 30, 2020

dyladan commented Jul 30, 2020

dyladan commented Jul 30, 2020

EdZou commented Jul 30, 2020 • edited Loading

EdZou commented Jul 21, 2020 •

edited

Loading

codecov bot commented Jul 21, 2020 •

edited

Loading

obecny left a comment •

edited

Loading

EdZou commented Jul 22, 2020 •

edited

Loading

EdZou commented Jul 22, 2020 •

edited

Loading

EdZou commented Jul 30, 2020 •

edited

Loading