Skip to content
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.

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

feat: add baggage support to the opentracing shim #918

Merged
merged 9 commits into from
Jul 27, 2020

Conversation

rubenvp8510
Copy link
Contributor

@rubenvp8510 rubenvp8510 commented Mar 31, 2020

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

Which problem is this PR solving?

Short description of the changes

  • Added baggage support for the opentracing shim.

@mayurkale22
Copy link
Member

@bg451 Would be nice if you can review this one.

@dyladan
Copy link
Member

dyladan commented Mar 31, 2020

This looks like it just stores the baggage directly as a correlation context entry, making it impossible to distinguish between our keys and baggage-shim keys. Is there any spec for this yet?

@rubenvp8510
Copy link
Contributor Author

I haven't found anything on related on the spec but saw how other clients (specifically opentelemetry-java) implemented this. may be I miss understood something?

@dyladan
Copy link
Member

dyladan commented Apr 1, 2020

I don't think you've misunderstood, I just expected there to be some specification to ensure all the various clients would work together cohesively. I'll review this today or tomorrow.

@rubenvp8510
Copy link
Contributor Author

Any thoughts about this? Should I modify the implementation?

Thanks.

@dyladan
Copy link
Member

dyladan commented Apr 29, 2020

I'm sorry the baggage PRs are waiting on spec issues which have been sitting around for a while. This should be resolved soon I hope.

@dyladan dyladan added this to In progress PRs in GA Burndown Jul 15, 2020
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this has sat for so long without any attention. Apart from the minor conflict, I think this looks like good work.

@open-telemetry/javascript-approvers can we please get some eyes on this? It is a small PR and should be no trouble to merge.

@rubenvp8510 if you don't want to or dont have time to resolve the conflicts, I can resolve them for you. Your contribution history will be maintained.

GA Burndown automation moved this from In progress PRs to Approved Jul 15, 2020
@rubenvp8510
Copy link
Contributor Author

@dyladan No problem, I can solve the conflicts and will update the PR.

Thanks!

@dyladan dyladan moved this from Approved to In progress PRs in GA Burndown Jul 15, 2020
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 17, 2020

CLA Check
The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #918 into master will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
+ Coverage   93.30%   93.53%   +0.23%     
==========================================
  Files         144      148       +4     
  Lines        4030     4144     +114     
  Branches      816      840      +24     
==========================================
+ Hits         3760     3876     +116     
+ Misses        270      268       -2     
Impacted Files Coverage Δ
...ackages/opentelemetry-shim-opentracing/src/shim.ts 88.00% <100.00%> (+3.74%) ⬆️
...ackages/opentelemetry-exporter-zipkin/src/types.ts 100.00% <0.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 98.46% <0.00%> (ø)
...ackages/opentelemetry-exporter-zipkin/src/utils.ts 100.00% <0.00%> (ø)
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100.00% <0.00%> (ø)
packages/opentelemetry-tracing/src/Span.ts 100.00% <0.00%> (+1.25%) ⬆️

@rubenvp8510
Copy link
Contributor Author

Rebased and added the extraction/injection of the baggage.

@rubenvp8510 rubenvp8510 requested a review from dyladan July 17, 2020 19:59
@dyladan dyladan self-requested a review July 17, 2020 21:43
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks for being patient with us.

@dyladan
Copy link
Member

dyladan commented Jul 20, 2020

@open-telemetry/javascript-approvers this has been a long time waiting and I would like to get it reviewed as it should be a fairly uncontroversial change.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Copy link
Contributor Author

Also added some propagation tests.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you can update https://github.com/open-telemetry/opentelemetry-js#release-schedule (Support for Tags/Baggage) with Beta v0.10.0 and today's date.

@dyladan could you please include this in proposed release PR?

packages/opentelemetry-shim-opentracing/src/shim.ts Outdated Show resolved Hide resolved
packages/opentelemetry-shim-opentracing/src/shim.ts Outdated Show resolved Hide resolved
GA Burndown automation moved this from In progress PRs to Approved Jul 23, 2020
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@dyladan
Copy link
Member

dyladan commented Jul 24, 2020

@dyladan could you please include this in proposed release PR?

sure no problem

@dyladan dyladan added the enhancement New feature or request label Jul 27, 2020
@dyladan
Copy link
Member

dyladan commented Jul 27, 2020

All 3 maintainers have approved I think this can be merged. @obecny @mayurkale22

@mayurkale22 mayurkale22 merged commit 5f859d1 into open-telemetry:master Jul 27, 2020
GA Burndown automation moved this from Approved to Done Jul 27, 2020
sidharthv96 added a commit to sidharthv96/opentelemetry-js that referenced this pull request Jul 28, 2020
* upstream/master:
  fix: add missing grpc-js index (open-telemetry#1358)
  chore: 0.10.0 release proposal (open-telemetry#1345)
  Proto update to latest to support arrays and maps (open-telemetry#1339)
  feat: add OTEL_LOG_LEVEL env var (open-telemetry#974)
  Add nodejs sdk package (open-telemetry#1187)
  feat: add baggage support to the opentracing shim (open-telemetry#918)
  Removing default span attributes (open-telemetry#1342)
jonahrosenblum pushed a commit to jonahrosenblum/opentelemetry-js that referenced this pull request Aug 8, 2020
Co-authored-by: Mayur Kale <mayurkale@google.com>
obecny added a commit that referenced this pull request Aug 17, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
GA Burndown
  
Done
Development

Successfully merging this pull request may close these issues.

Add baggage support to the opentracing shim
4 participants