opentracing / opentracing-javascript Goto Github PK

@bcronin My npm user name is "oibe" can you give me permissions to publish?

As I understand it the minimal required code to trace a method is this:

async function foo(parent) {
  const span = parent.tracer().createSpan('foo', { childOf: parent })
  try {
    // do work
  } catch (err) {
    span.set('error', true);
    span.log({ event: 'error', 'error.object': err })
    throw err;
  } finally {
    span.finish();
  }
}

const span = globalTracer().createSpan(‘bar’)
foo(span)

That try/catch/finally dance is quite verbose and prone to human error (forgetting the catch for example).

I think we need a simpler API like this:

function foo(parent) {
  return parent.trace('foo', async () => {
    // do work
  });
}

That automatically sets error tags and finishes spans when the given function completed.

(Copy-and-paste of a pull request comment below)

FYI, @bensigelman making this into a separate issue since the PR was already merged.

@bensigelman , I might be wrong, but I'm not sure the signature is going to work for JavaScript:

inject(span, format, carrier)

...where carrier is an ArrayBuffer.

ArrayBuffers are fixed-length at construction time. This signature would require the caller to know the size of the buffer the implementation needs -- which, unless there's something I'm missing, is not a reasonable expectation :)

This could easily be fixed by having inject return a carrier rather than have one passed in. I'm not sure if this is a "valid" platform-specific variation from the semantic spec or not.

Another option is that the binary carrier is defined as a plain Object with a single buffer field that the implementation sets.

I'm not aware of a native resizable binary buffer type that's broadly available in browsers that could be used in place of the fixed-length Array types to keep the inject signature as-is.

This is the method signature for span.finish

finish(finishTime?: number): void

This signature does not accept a callback nor is it synchronous. Hence, if there is an error reporting the span, an unhandled exception will be thrown and the node process will die. The current workaround is to watch on the unhandledException event as follows:

process.on('uncaughtException', err => {
  console.error(err)
})

This seems like a very huge gap and so I'm wondering why the method signature for span.finish does not support callbacks.

Additionally, is there a different way to handle errors that might be caused during the execution of span.finish that I am missing.

If indeed this is a bug, I can propose a solution and PR a fix. Perhaps something like:

finish(finishTime?: number, callback?: function): void

span.finish(err => console.log(err))

This is a meta issue, probably as a follow up from #110.

To provide better support for web client tracing, it would be nice to understand what are the current stacks/frameworks being used and how we could transparently instrument them. Are there hooks we can add listeners/filters/interceptors?

The first part of this task is to identify the frameworks/stacks eligible for instrumenting. For each framework, open a new issue and link to this one.

The code should add a tool to do basic API compliance testing against an arbitrary backend. This is particular useful in a dynamic language where a compiler will not catch basic function signature deviations, etc.

As above there is no dist/ folder containing .js and .min.js package as one would think when reading "Browser Usage" in readme.md.

Consider the following scenario in Node:

• Application A has a dependency on package B
• A has a dependency on [email protected]
• B has a dependency on [email protected]

All trace data recorded with in package B should be reported to (and be compatible) with the implementation set up in Application A.

This situation contains quite a few subtleties. For example, there will be two versions of opentracing within the global application and they need to ensure they share the same tracing implementation object (so data is all funneled to the same place). Explicitly ensuring this scenario is accounted for would be good since it's (unsurprisingly) very difficult to correct forwards-compatibility issues after the fact.

The best way to solve this would be to remove the dependency on underscore, but otherwise the library should depend on underscore.

Mock tracer has no tests which is important for something which is used to test other components.

The SpanOptions type is not exported from the main file. This means implementations cannot use this type.

https://github.com/opentracing/opentracing-javascript/blob/master/src/index.ts#L7

The documentation in global_tracer says:

// Allows direct importing/requiring of the global tracer:
//
// let globalTracer = require('opentracing/global');
//      OR
// import globalTracer from 'opentracing/global';

but opentracing/global doesn't actually exist. Is there any reason we have to call a function to get the global tracer, instead of just importing it? And do we actually need a Singleton tracer, can't we just pass Spans around?

To bring JS API in parity with other languages, per opentracing/opentracing.io#106

This code is intended to build packages for both Node.js and the browser. It's unclear how to correctly implement the "binary encoding" without making assumptions about the platform (i.e. Buffer, Uint8Array, etc.).

This does raise some concern that an implementation-detail may be creeping into the API.

The circular dependency of Tracer -> noop -> Tracer is definitely a code smell that is worked around here with an initialize function that needs to be called. Why not simply remove the noop.js file, and let the tracer, span etc files export a noop instance (if we need an eagerly instantiated instance at all)?

Right now Mock Tracer is not exported, although it is part of the npm package. Should we remove it from the npm package or export it. Otherwise people will need to use it with a direct file system reference into node_modules which is weird.

From @syrnick in #24 :

This causes a global leak:

Error: global leak detected: __opentracing_singleton (when running tests with mocha --check-leaks ...)

In node, the libraries should not use global namespace, module itself is a singleton, so it's a good place to stash the singleton. Sometimes it's tricky to ensure that the module is really a singleton due to the relative requires, but then it's up to the consumer to ensure that it's required consistently.

Maybe this: http://dev.topheman.com/es6-code-coverage-with-babel-plugin/

The compiling makes debugging really difficult.

It seems like the basic tasks of compilation would be expressed equally well with npm scripts, which Javascript developers are way more familiar with and already have node_modules/.bin in their PATH. make is also not available on Windows.

The repo should include a trivial backend implementation to make it obvious how to implement a compliant-OpenTracing JS tracing backend.

What is your take on releasing version 1.0.0 of this module? I believe it would strengthen peoples' take on opentracing's API stability. Also, it would fit the specification's version more closely.

@bcronin @bensigelman I've checked with our local JS experts and they suggested a different pattern for defining the API & implementation, which makes more sense to me. The example is:

• interface https://github.com/uber-common/larch/blob/master/base-backend.js
• implementation https://github.com/uber-common/larch/blob/master/logtron-backend.js

The assertions can be removed from prod version via the same pattern already used in this repo, because while they are ok in the above example (where the backend is created only once), they will be expensive for every span/context creation.

All of the other opentracing languages have tags defined. Since javascript is not strictly type checked I don't know if its necessary to have anything more than constants, but its still something we need.

The JavaScript implementation should follow suite of languages like Java and Python and implement the proposed ScopeManager specification. This would make managing the active span significantly easier.

RFC: https://github.com/opentracing/specification/blob/master/rfc/scope_manager.md
Python: opentracing/opentracing-python#63
Java: opentracing/opentracing-java#188

The webpack / babel configuration for production should set the API_CONFORMANCE_CHECKS constant to false and ensure the resulting dead code is remove (for performance reasons).

This is not yet implemented.

The base eslint rules configuration doesn't allow for version 3.x of eslint (even though it is compatible). Fixing this is likely just a matter up updating some package versions for the rules config files.

I think it'd be a bit cleaner to name it opentracing-js, no? just a thought

Would be helpful (even if it's a small list today) to see direct links to known tracer implementations in readme.md.

Could we get a gitter going for general OT JS community discussion, similar to opentracing/opentracing-python?

Node 0.12 entered maintenance on 2015-10-01 and reached end of life on 2016-10-31.
Latest Node version is v7, current LTS is v6 and v4 is LTS until 2018. Many libraries dropped support for Node 0.12, for example TSLint, preventing the usage for us. I think moving to Node 4 and following the LTS schedule would be reasonable.

Mock tracer could be incredibly useful for unit tests. I saw a TODO for moving it to its own NPM package but I think it'd be more useful if exposed here. If you think it's a good idea, I'd be happy to make a PR and take ownership of its maintenance.

What are your thoughts?

I'm trying to write an implementation, but I'm not sure how to go about implementing follows_from.

It looks like this isn't implemented in startSpan() in tracer.js.

I've had a read through the git history and it looks like this may have been implemented at some point.

Can someone show how this should be done in an implementation, as well as an example of how to use it in practice.

I would expect it to work like this, based on how childOf works.

const parentSpan = tracer.startSpan('parentOperation');
const childSpan = tracer.startSpan('childOperation', { followsFrom: parentSpan });

Thanks!

Either one of two (backwards non-compatible) changes would make things easier for implementers:

• Have the default Tracer.startSpan implementation call Span.addTags after calling the Span constructor.
• Specify that addTags has no effect if passed a falsy value, and have the default Span.addTags implementation perform this check. Then the Span constructor can just call this.addTags(fields && fields.tags).

https://doc.esdoc.org/

I think we should update the module references to get rid of warnings

npm WARN deprecated [email protected]: ⚠️ WARNING ⚠️ tar.gz module has been deprecated and your application is vulnerable. Please use tar module instead: https://npmjs.com/tar
npm WARN deprecated [email protected]: to-iso-string has been deprecated, use @segment/to-iso-string instead.
npm WARN deprecated [email protected]: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: Use uuid module instead
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: please upgrade to graceful-fs 4 for compatibility with current and future versions of Node.js

To generate the lib-debug version with TypeScript and remove the type checks from the lib version the build step would have to become a lot more complicated (probably a gulp task). Do we really need this differentiation? Imo either we should either leave the type checks in (Checking some variables with typeof shouldn't be a real performance problem) or remove them completely and let users use TypeScript if they want (compile-time) type safety.

What are the plans to release a new version? The last release was 10 months ago and master has quite a few improvements since that time.

Basic usage of the OpenTracing APIs in JS should be provided along with the code.

@bcronin
Can you release a new version that contains tags, and debug-lib?

@bcronin @bensigelman
1.) In apiCompatibilityChecks the "createTracer" function is never used, so that means I can't import these tests as a client, since my tracer will never be applied.
2.) The tests assert that errors are thrown when certain conditions are not met, and that certain types are passed in. My issue with this is that it may be dictating the responsibility of the implementation too much. For example at Uber a lot of our clients do not throw exceptions, because they would require try/catch which deoptimizes the v8 interpreter, and also there is usually nothing you can do about some of these errors. (sidenote: There is a larger solution in place so apps, don't crash.)

Essentially I would like to be able to use opentracing compatibility tests without the additional restrictions of typechecking, and error's being thrown.

When running the example I get the following error:

npm run example

> [email protected] example /Users/cwat-areitbau/workspace/attic/node_modules/opentracing
> node lib/examples/demo/demo.js

module.js:471
    throw err;
    ^

Error: Cannot find module 'lodash'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/cwat-areitbau/workspace/attic/node_modules/opentracing/lib/mock_tracer/mock_span.js:14:9)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

npm ERR! Darwin 17.4.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "run" "example"
npm ERR! node v6.9.5
npm ERR! npm  v3.10.10
npm ERR! code ELIFECYCLE
npm ERR! [email protected] example: `node lib/examples/demo/demo.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] example script 'node lib/examples/demo/demo.js'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the opentracing package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node lib/examples/demo/demo.js
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs opentracing
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls opentracing
npm ERR! There is likely additional logging output above.
npm WARN Local package.json exists, but node_modules missing, did you mean to install?

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/cwat-areitbau/workspace/attic/node_modules/opentracing/npm-debug.log

From Yuri Shkuro:

we could do it in different PR or in this one, but it seems none of the functions are actually being tested. For example, I would expect tests like:

each method should be called at least once by the harness
tracer() returns something that has the Tracer methods on it
methods like setTag() return something on which setTag can be called again (chaining)
get-baggage returns can return what was set with set-baggage (in Python we made this controlled by a parameter to the harness because noop implementation does not store the baggage)
Before we use this harness with our tracer, we need these tests implemented.

The API documentation suggests that the purpose is to allow a single-argument call to tracer.startSpan

tracer.startSpan({
    operationName: "makeRPC",
    tags: {...}
});

but the code doesn't actually support this single-argument setup.

According to the readme on this repo, web client is supported, but it lacks some examples or further documentation on what's known to work, including which tracers could be used for web clients (see #108)

This task is a research task for an initial support for web clients:

• Start a new application using some common stack/framework
• Use the library in this project to perform some manual instrumentation
• Use the MockTracer to assert that the instrumentation works

The end result of this task should be made a reference of how to do tracing with web clients, without taking security into consideration

I was testing 0.14 with jaeger-client-node, and ran into the following issue. Our client is written in ES6 and transpiled to JS with babel.

https://github.com/uber/jaeger-client-node/blob/master/test/udp_sender.js

// L28
import opentracing from 'opentracing';
// L124
let childOfRef = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, childOfContext);

compiled to

var _opentracing = require('opentracing');
var _opentracing2 = _interopRequireDefault(_opentracing);
// ...
var childOfRef = new _opentracing2.default.Reference(_opentracing2.default.REFERENCE_CHILD_OF, childOfContext);

When running throws this error:

        var childOfRef = new _opentracing2.default.Reference(_opentracing2.def
                                                  ^
TypeError: Cannot read property 'Reference' of undefined

@felixfbecker - any idea?

I wonder why lib / dist are commited to GitHub. Usually you only publish these on NPM, just like you wouldn't commit a compiled binary to GitHub, or node_modules.

The current documentation says to use this snippet to run API compatibility tests:

const apiCompatibilityChecks = require('opentracing/test/api_compatibility.js');
apiCompatibilityCheck(() => new CustomTracer());

However, since version 0.14, it should be instead:

const apiCompatibilityChecks = require('opentracing/lib/test/api_compatibility.js').default;
apiCompatibilityCheck(() => new CustomTracer());

See: https://github.com/opentracing/opentracing-javascript#api-compatibility-testing