Scope #117

rochdev · 2018-12-10T20:16:00Z

This PR implements the scope manager specification using continuation passing style as discussed in opentracing/specification#126 and #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.

coveralls · 2018-12-12T21:35:17Z

Coverage decreased (-2.2%) to 86.349% when pulling 0e21f2c on rochdev:scope into 3b03e8e on opentracing:master.

coveralls · 2018-12-12T21:35:19Z

Coverage decreased (-2.2%) to 86.349% when pulling 0e21f2c on rochdev:scope into 3b03e8e on opentracing:master.

coveralls · 2018-12-12T21:35:20Z

Coverage decreased (-2.2%) to 86.349% when pulling 0e21f2c on rochdev:scope into 3b03e8e on opentracing:master.

rochdev · 2019-01-17T15:30:35Z

src/scope.ts

+    }
+
+    protected _active(): Span | null {
+        return null;


Should a no-op span be returned here instead? It would have the benefit of not breaking the app if a user disables the tracer in situations where the code assumes there is always a span available for a specific scope.

Conventionally, active returns either the current Scope or null.

felixfbecker

I don't fully understand what this PR enables a user, as it doesn't seem to actually implement any continuation passing through e.g. async hooks. 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. The PR description says it's up to vendors, but why should e.g. Jaeger and Lightstep have to implement these?

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?

felixfbecker · 2019-01-17T15:38:31Z

src/@types/emitter-listener.d.ts

@@ -0,0 +1,20 @@
+declare module 'emitter-listener' {


Could you submit this to DefinitelyTyped instead?

Yes, but in the meantime it should not block this PR.

felixfbecker · 2019-01-17T15:41:26Z

src/tracer.ts

+     * boolean to indicate that the childOf option should not implicitly be set
+     * to the currently active span.
+     */
+    ignoreActiveSpan?: boolean;


If this defaults to false, is this a breaking change?

It's breaking in that a span may end up with a parent where there were no parent before. I personally don't mind either way, but according to the spec it should be false by default.

felixfbecker · 2019-01-17T15:41:46Z

tsconfig.json

+    "module": "commonjs",
+    "baseUrl": ".",
+    "paths": {
+        "*": ["src/@types/*"]


This should be done with typeRoots

I'm not very familiar with TypeScript. Could you explain the difference?

felixfbecker · 2019-01-17T15:41:56Z

tsconfig.json

@@ -2,19 +2,24 @@
  "compilerOptions": {
    "target": "es5",
    "lib": [
-      "es5",
+      "es2015",


This is a breaking change

Could you explain why this is a breaking change? My understanding is that this is necessary to support using Promise, and doesn't change the output, only the linting.

felixfbecker · 2019-01-17T15:43:10Z

src/scope.ts

+     *
+     * @returns {any} The provided function bound to the span.
+     */
+    bind<T>(fn: ScopeCallback<T>, span?: Span | null): ScopeCallback<T>;


Doesn't this overload throw away the types of the parameters?

Could you elaborate? I haven't done a lot of TypeScript so any help with typing is appreciated.

rochdev · 2019-01-18T14:40:26Z

@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.

felixfbecker · 2019-01-18T14:46:24Z

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

rochdev · 2019-01-19T03:34:52Z

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.

rochdev · 2019-01-22T14:54:49Z

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.

rochdev · 2019-02-08T18:04:41Z

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?

rochdev · 2019-04-24T18:32:49Z

@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.

yurishkuro · 2019-04-24T22:30:03Z

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).

rochdev · 2019-04-24T22:37:43Z

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?

yurishkuro · 2019-04-24T22:54:48Z

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.)?

rochdev · 2019-04-24T23:01:48Z

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.

first implementation with only noop + update tooling

2bcf6cf

rochdev force-pushed the scope branch from b8da386 to 2bcf6cf Compare December 12, 2018 21:25

rochdev added 2 commits December 12, 2018 16:28

code cleanup

9ac19d3

code cleanup

0e21f2c

rochdev added 4 commits December 12, 2018 16:51

fix older node versions

d3af338

update bind to support more types

2f29b13

add tests for binding

06514f3

fix tests in node 0.10

3e89c90

rochdev changed the title ~~[WIP] Scope~~ Scope Jan 16, 2019

code cleanup

158c371

rochdev commented Jan 17, 2019

View reviewed changes

felixfbecker reviewed Jan 17, 2019

View reviewed changes

remove base implementation for scope.bind()

fdea8c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scope #117

Scope #117

rochdev commented Dec 10, 2018 •

edited

Loading

coveralls commented Dec 12, 2018

coveralls commented Dec 12, 2018

coveralls commented Dec 12, 2018

rochdev Jan 17, 2019

austinlparker Jan 17, 2019

felixfbecker left a comment

felixfbecker Jan 17, 2019

rochdev Jan 18, 2019

felixfbecker Jan 17, 2019

rochdev Jan 18, 2019

felixfbecker Jan 17, 2019

rochdev Jan 18, 2019

felixfbecker Jan 17, 2019

rochdev Jan 18, 2019

felixfbecker Jan 17, 2019

rochdev Jan 18, 2019

rochdev commented Jan 18, 2019

felixfbecker commented Jan 18, 2019

rochdev commented Jan 19, 2019

rochdev commented Jan 22, 2019

rochdev commented Feb 8, 2019

rochdev commented Apr 24, 2019

yurishkuro commented Apr 24, 2019

rochdev commented Apr 24, 2019

yurishkuro commented Apr 24, 2019

rochdev commented Apr 24, 2019

Scope #117

Are you sure you want to change the base?

Scope #117

Conversation

rochdev commented Dec 10, 2018 • edited Loading

coveralls commented Dec 12, 2018

coveralls commented Dec 12, 2018

coveralls commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochdev commented Jan 18, 2019

felixfbecker commented Jan 18, 2019

rochdev commented Jan 19, 2019

rochdev commented Jan 22, 2019

rochdev commented Feb 8, 2019

rochdev commented Apr 24, 2019

yurishkuro commented Apr 24, 2019

rochdev commented Apr 24, 2019

yurishkuro commented Apr 24, 2019

rochdev commented Apr 24, 2019

rochdev commented Dec 10, 2018 •

edited

Loading