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

Difference between module Readmes and examples #831

Closed
kriptontr opened this issue Jan 13, 2022 · 5 comments
Closed

Difference between module Readmes and examples #831

kriptontr opened this issue Jan 13, 2022 · 5 comments

Comments

@kriptontr
Copy link

Usage of registerInstrumentations is like

registerInstrumentations({
  instrumentations: [
    // Express instrumentation expects HTTP layer to be instrumented
    new HttpInstrumentation(),
    new ExpressInstrumentation(),
  ],
});

at https://www.npmjs.com/package/@opentelemetry/instrumentation-express
but its used like

  registerInstrumentations({
    tracerProvider: provider,
    instrumentations: [
      // Express instrumentation expects HTTP layer to be instrumented
      HttpInstrumentation,
      ExpressInstrumentation,
    ],
  });

without getting instances of classes here.
https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/examples/express/tracer.js

  • [+] This only affects the JavaScript OpenTelemetry library
  • [+] This may affect other libraries, but I would like to get opinions here first
@Flarna
Copy link
Member

Flarna commented Jan 13, 2022

Both works.
The implementation of instrumentation checks if a function/constructor is passed and calls new in that case, otherwise it uses the given instance. see here

Personally I would prefer to avoid such magic but I doubt that this will be removed as it would most likely break some user code.

@dyladan
Copy link
Member

dyladan commented Jan 13, 2022

Both works. The implementation of instrumentation checks if a function/constructor is passed and calls new in that case, otherwise it uses the given instance. see here

Personally I would prefer to avoid such magic but I doubt that this will be removed as it would most likely break some user code.

I would also prefer not to have it. It would break user code, but technically it isn't 1.0 yet so it's not too late officially. With tracing being 1.0 we might be stuck with it anyway though.

@legendecas
Copy link
Member

legendecas commented Jan 14, 2022

Might be off-topic: This reminds me of the inconsistency of the document with the actual implementation of the autoloading of instrumentation: open-telemetry/opentelemetry-js#2410 (comment) & open-telemetry/opentelemetry-js#1989. If we are going to update the instrumentation API, I'd think they should be considered too.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Mar 21, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

This issue was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this as completed Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants