opentracing
Scope
This PR implements the scope manager specification using continuation passing style as discussed in https://github.com/opentracing/specification/issues/126 and https://github.com/opentracing/opentracing-javascript/issues/112.
The implementation assumes a scope is always available and thus doesn't require an actual scope manager. The scope itself handles creating child scopes and activating spans.
An utility method for binding different types of callback constructs has been included as well. Right now it supports callback functions, promises and event emitters, but the API is flexible and could be extended with additional constructs eventually.
For now only a no-op implementation is provided, and the actual implementation is up to the vendors. In the future, implementations should live in this repository since this is a cross-cutting concern that is always handled the same way.
Coverage decreased (-2.2%) to 86.349% when pulling 0e21f2c27eb673109fc35d5235dac567bfb2d38e on rochdev:scope into 3b03e8eb97894634e88c73abbfb4d49d1a185a34 on opentracing:master.
Coverage decreased (-2.2%) to 86.349% when pulling 0e21f2c27eb673109fc35d5235dac567bfb2d38e on rochdev:scope into 3b03e8eb97894634e88c73abbfb4d49d1a185a34 on opentracing:master.
Coverage decreased (-2.2%) to 86.349% when pulling 0e21f2c27eb673109fc35d5235dac567bfb2d38e on rochdev:scope into 3b03e8eb97894634e88c73abbfb4d49d1a185a34 on opentracing:master.
@felixfbecker Thanks for the review!
It seems like there is still manual code needed to propagate the spans, so there doesn't seem to be an improvement over the status quo.
Right now it's not possible for vendors to implement an OpenTracing scope manager since there is no interface, meaning that right now every vendor has completely different APIs and implementations. This PR enables vendors to start moving to the official API and to eventually contribute this repo. It also enables the implementations to be compatible with OpenTracing by exposing official interfaces. I focused on only the minimum required functionality for this PR, and then we can iterate.
The PR description says it's up to vendors, but why should e.g. Jaeger and Lightstep have to implement these?
The idea is to eventually have all the implementations in sub-projects of this repo. For example, I have an async_hooks implementation ready to go. However, trying to move this repo to a multi project monorepo proved to be too big of a change in one go, so I think this should be done in iterations. It will also allow different vendors with different expertise to contribute to different sub-projects. As an example, we don't currently support the browser so it wouldn't make sense for us to contribute an implementation for the browser given our lack of expertise on the subject.
It also seems to favour specific async representations - Node EventEmitters and Promises, but what about usage in the browser with EventTargets, or Observables, Genertors, AsyncIterables, etc?
This is why I made the bind() method extensible. It makes it easy to add new target types, especially future types that are not yet known. I added the most common ones to start but we can iterate later with other types. Vendors that have their own custom types will also be able to implement these types.
tl;dr This PR is the minimum functionality required so that vendors can start implementing scope managers compatible with OpenTracing. We can then iterate with future PRs to have the different implementations live directly in this repo.
meaning that right now every vendor has completely different APIs and implementations.
Could you link an example of a vendor API for scope management? I've only used Jaeger and LightStep which don't have one afaik
we don't currently support the browser
Do you mean just the new code in this PR? Because I am using the current version of opentracing in production in the browser
This is why I made the bind() method extensible. It makes it easy to add new target types, especially future types that are not yet known. I added the most common ones to start but we can iterate later with other types. Vendors that have their own custom types will also be able to implement these types.
What do you mean by "extensible"? Being a method of a class it seems rather sealed to me? It would also mean that even if I don't use e.g. Observables in my project, the code for supporting Observables cannot be tree-shaked.
So far I've used simple helper functions to manage spans: https://sourcegraph.com/github.com/sourcegraph/javascript-typescript-langserver@64d9479c89039290f9aa530547bf2845e6d33c49/-/blob/src/tracing.ts#L35
Could you link an example of a vendor API for scope management? I've only used Jaeger and LightStep which don't have one afaik
I know at least New Relic, Elastic APM, Stackdriver Trace and OpenCensus each do it their own way. I don't know if they have OpenTracing compatibility however, especially since having your own scope manager automatically makes you partially incompatible with OpenTracing right now.
Do you mean just the new code in this PR? Because I am using the current version of opentracing in production in the browser
I mean that we at Datadog don't currently support the browser, which is why I focused on Node for now. As mentioned however, additional browser specific functionalities can be added in future iterations.
What do you mean by "extensible"? Being a method of a class it seems rather sealed to me?
Right now by simply implementing the _bind() protected method it's possible to add additional binding methods in subclasses.
It would also mean that even if I don't use e.g. Observables in my project, the code for supporting Observables cannot be tree-shaked.
That could be optimized eventually by adding a way to register additional binding methods externally, but for now I only included the functionalities that would be required for every single vendor meaning that this is currently a non-issue. It's also worth noting that each additional method is usually around 10 lines of code, so IMHO the pros outweighs the cons.
So far I've used simple helper functions to manage spans
The problem with this is that vendors can only implement existing methods, not add new ones. This is why I hide the different implementations behind a single method.
Removed the implementation for scope.bind() to keep the base class as strictly no-op. This will allow vendors to implement it according to their needs, and eventually we can backport any relevant code directly to this repo.
@austinlparker @felixfbecker Can you take another look?
@yurishkuro What should be done with this PR? Should we close it? There is a clear lack of interest to merge this feature and OpenTracing is scheduled to be deprecated by the upcoming merge of OpenTracing and OpenCensus.
I would keep it, if only as a reference. You're correct, it's hard to allocate resources to this given the OT/OC merger and the work on the new libs. It would be great if you could participate in those, and perhaps transfer this PR into the new project (once it's set up).
I will definitely keep an eye open for the new project and port the scope manager if needed.
@yurishkuro What is the best place right now to track the current status of the merger? Any way I can help for JavaScript?
Updates / schedules are being posted on Medium/Twitter. https://medium.com/opentracing/a-roadmap-to-convergence-b074e5815289
The work on Javascript hasn't started yet.
@tedsuo thoughts? Do we plan to kick off other languages before Java API is solidified (naming, etc.)?
I would argue it’s probably best to do at least 1 dynamic interpreted language at the same time as Java, as in my experience APIs designed solely for Java tend to ignore fundamental differences with other languages. This also applies to terminology and async patterns. In general I find Python and JavaScript to be good candidates to find such discrepancies.