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

NodeSDK should register instrumentations earlier #3146

Closed
Flarna opened this issue Aug 5, 2022 · 5 comments · Fixed by #3502
Closed

NodeSDK should register instrumentations earlier #3146

Flarna opened this issue Aug 5, 2022 · 5 comments · Fixed by #3502
Assignees
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@Flarna
Copy link
Member

Flarna commented Aug 5, 2022

Is your feature request related to a problem? Please describe.

Instrumentations need to be registered before the corresponding module to instrument is loaded. Currently NodeSDK calls registerInstrumentations() as last step in start() (see here). This might lead to missing some modules to instrument es e.g. resource detection does already HTTP requests before.

Describe the solution you'd like

We should move registerInstrumentations() call to the beginning of start or even into constructor. Or maybe even remove this functionality from SDK and require user to do it before require the SDK.

Describe alternatives you've considered

Additional context

refs.: open-telemetry/opentelemetry-js-contrib#1109 (comment)

@dyladan dyladan added bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect and removed feature-request labels Aug 5, 2022
@dyladan
Copy link
Member

dyladan commented Aug 5, 2022

I'm actually going to reclassify this as a bug since it definitely can cause problems. The node sdk package is something that was done experimentally a long time ago but now has gained quite some users and should be brought to the level of quality they expect

@dyladan dyladan self-assigned this Aug 10, 2022
@AWare
Copy link

AWare commented Aug 11, 2022

As an alternative, can I suggest that instrumentation be a pre-runtime step? Having all the instrumentations patched into node_modules where possible would really help with reliability- especially where bundlers are used.

@Flarna
Copy link
Member Author

Flarna commented Nov 10, 2022

@AWare What do you mean in detail with pre-runtime step?

@Flarna Flarna added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Dec 21, 2022
@Flarna Flarna removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Dec 21, 2022
@Flarna
Copy link
Member Author

Flarna commented Dec 21, 2022

@dyladan Even it's assigned to you I created a simple PR to improve this: #3502

@dyladan
Copy link
Member

dyladan commented Dec 21, 2022

Thanks @Flarna I haven't had time to work on this so I appreciate that you handled it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants