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

Create meta package with default instrumentations so they can be passed automatically #1824

Closed
obecny opened this issue Jan 14, 2021 · 11 comments

Comments

@obecny
Copy link
Member

obecny commented Jan 14, 2021

Something like this

const { registerInstrumentations } = require('@opentelemetry/instrumentation');
const defaultNodeCoreInstrumentations = require('@opentelemetry/instrumentation-core-node-defaults');

registerInstrumentations({
  instrumentations: [
    defaultNodeCoreInstrumentations,
  ],
});

@dyladan
Copy link
Member

dyladan commented Jan 14, 2021

Is instrumentations a list of lists?

@obecny
Copy link
Member Author

obecny commented Jan 14, 2021

https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-instrumentation/src/types_internal.ts#L26

so this can be array of InstrumentationBase or array of Instrumentation etc.

@dyladan
Copy link
Member

dyladan commented Jan 14, 2021

Why not flatten the list though?

const { registerInstrumentations } = require('@opentelemetry/instrumentation');
const defaultNodeCoreInstrumentations = require('@opentelemetry/instrumentation-core-node-defaults');

registerInstrumentations({
  instrumentations: [
    ...defaultNodeCoreInstrumentations,
  ],
});

Also it seems like an unnecessary redirection to require the user to install 2 packages. Why not just do this?

const defaultNodeCoreInstrumentations = require('@opentelemetry/instrumentation-core-node-defaults');

// this options object can have useful types now :)
const options = {/* */};
defaultNodeCoreInstrumentations.enable(options);

@obecny
Copy link
Member Author

obecny commented Jan 14, 2021

too complicated. This must be compatible with already existing loader so you provide them the same way you would do it manually when adding package one by one including old plugins etc.

const { registerInstrumentations } = require('@opentelemetry/instrumentation');
const defaultNodeCoreInstrumentations = require('@opentelemetry/instrumentation-core-node-defaults');
const defaultNodeContribInstrumentations = require('@opentelemetry/instrumentation-core-node-defaults');
const { GraphQLInstrumentation } = require('@opentelemetry/instrumentation-graphql');

registerInstrumentations({
  instrumentations: [
    defaultNodeCoreInstrumentations,
    defaultNodeContribInstrumentations,
    GraphQLInstrumentation,
    ....
  ],
});

@dyladan
Copy link
Member

dyladan commented Jan 14, 2021

Why would it be too complicated?

const defaultNodeCoreInstrumentations = require('@opentelemetry/instrumentation-core-node-defaults');
const defaultNodeContribInstrumentations = require('@opentelemetry/instrumentation-contrib-node-defaults');
const { GraphQLInstrumentation } = require('@opentelemetry/instrumentation-graphql');

defaultNodeCoreInstrumentations.enable();
defaultNodeContribInstrumentations.enable();

new GraphQLInstrumentation().enable();

@obecny
Copy link
Member Author

obecny commented Jan 14, 2021

You aware that we already have a new autoloader registerInstrumentations which works as I mentioned and the only reason to have meta package is to have a list of default instrumentations and pass it, instead of manually adding each of the instrumentation. Your example is missing autoloader completely.

@dyladan
Copy link
Member

dyladan commented Jan 14, 2021

My example would call the autoloader "behind the scenes". No need for the user to call it themselves unless they have special requirements.

@obecny
Copy link
Member Author

obecny commented Jan 14, 2021

My example would call the autoloader "behind the scenes". No need for the user to call it themselves unless they have special requirements.

how ?, you are loading plugins and instrumentations once with registerInstrumentations, you are not doing this in many different places "behind scenes" user have full control over it.

@obecny
Copy link
Member Author

obecny commented Jan 14, 2021

and just a reminder of the thing that was implemented
#1731

@dyladan
Copy link
Member

dyladan commented Jan 14, 2021

I remember what was implemented, but I thought it was only temporary while we make the transition to instrumentations. Once we are using new instrumentations everywhere there is no need to have a loader anymore.

@vmarchaud
Copy link
Member

Closing since it has been implemented on the instrumentation package

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
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

3 participants