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

Instrumentation Autoload #1672

Closed
dyladan opened this issue Nov 11, 2020 · 11 comments · Fixed by #1731
Closed

Instrumentation Autoload #1672

dyladan opened this issue Nov 11, 2020 · 11 comments · Fixed by #1731
Assignees

Comments

@dyladan
Copy link
Member

dyladan commented Nov 11, 2020

Per the call today, we would like to be able to configure instrumentations like this:

const { registerInstrumentations } = require("@opentelemetry/instrumentation");
const instrumentations = require("@opentelemetry/instrumentations-node-core")

const MyPlugin = require("my-plugin") // old plugin
const MyInstrumentation = require("my-instrumentation") // new instrumentation

registerInstrumentations({
    instrumentations: [...instrumentations, MyPlugin, MyInstrumentation],
    // optional - use globals by default
    tracerProvider: ...,
    meterProvider: ...,
}),
@obecny obecny self-assigned this Dec 1, 2020
@Flarna
Copy link
Member

Flarna commented Dec 1, 2020

Should this be api.register()?
Or is sdk some new component?

@vmarchaud
Copy link
Member

@Flarna I recall that we didn't define where this should be exposed, althrough i believe this should be either on tracing/metrics sdk or on a highlever component like the current node-sdk

@dyladan
Copy link
Member Author

dyladan commented Dec 1, 2020

@Flarna the sdk component is just a wrapper around the sdk components you're already familiar with which handles startups with default options.

edit: as pointed out by @vmarchaud the code example here isn't really meant to be specific and is more about the loading pattern. It could very well be the tracer provider or something.

@Flarna
Copy link
Member

Flarna commented Dec 1, 2020

I think one of the targets of OTel is to keep the SDK interchangeable therefore it should be in some interface component an not in a concrete implementation of one SDK.

@dyladan
Copy link
Member Author

dyladan commented Dec 2, 2020

These are exclusive concepts. The instrumentation will still be useable by other sdks, and the "sdk module" is just a helper which wraps the api calls. Any sdk implementation would need to reimplement it. If a user specifically depends on @opentelemetry/node-sdk, they are not using a third party sdk.

@Flarna
Copy link
Member

Flarna commented Dec 2, 2020

Would it be possible to move this implementation into the instrumentation package? This would avoid that every SDK vendor has to re-implement the same functionality.

@dyladan
Copy link
Member Author

dyladan commented Dec 2, 2020

I don't see any reason why it couldn't live in the instrumentation package.

@dyladan
Copy link
Member Author

dyladan commented Dec 3, 2020

@obecny after this change is there any reason to have web and node packages? The plugin loading is the only difference and if we can move it into the instrumentation package then we can remove those packages entirely.

@obecny
Copy link
Member

obecny commented Dec 3, 2020

@dyladan are you referring to opentelemetry-node and opentelemetry-web ?

@dyladan
Copy link
Member Author

dyladan commented Dec 3, 2020

yes. With the new instrumentation it should be possible to remove them

@obecny
Copy link
Member

obecny commented Dec 8, 2020

In web there are some utils that are being used by few plugins. In node there is default context manager already defined which is quite convenient for the end user. Based on this I don't see a good place for utils from web and then forcing the user to choose the context manager everytime. Now you just make sure you get the correct TracerProvider and thats it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants