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

feat!: use modern diagnostics_channel API #2407

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

secondfry
Copy link

@secondfry secondfry commented Aug 28, 2024

Which problem is this PR solving?

I'm observing that instrumentation-undici does not work in Node.js 18.16.0 if code was started with no Internet access. I nailed it down to usage of channel.subscribe(onMessage) which was deprecated in Node.js 18.7.0.

Interestingly enough new API does not pass tests on all Node.js versions between 18.7.0 and 18.18.2. I think this is related to nodejs/node#47520. Thus I'm setting minimal engine version to 18.19.0.

Short description of the changes

Use diagnostics_channel.subscribe(name, onMessage) instead of channel.subscribe(onMessage).

@david-luna
Copy link
Contributor

Hi @secondfry

thanks for contributing to opentelemetry :)

IMHO the change you propose is the way to go since the API is deprecated but I would prefer not to drop support for node versions if possible. Have you thought to use the diagnostics_channel.subscribe API if available and fallback to channel.subscribe if not? this logic on which API should be used can also check for specific node versions if there is a known issue as you mention.

Cheers

@david-luna
Copy link
Contributor

@secondfry did you have time to think about my suggestion on nodejs support?

@david-luna
Copy link
Contributor

Hi @secondfry

If you have no bandwidth I can take this one although I'd go for not dropping support of Node.js versions

@david-luna
Copy link
Contributor

@secondfry this PR includes your proposal along a few changes to keep compatibility
#2457

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

Successfully merging this pull request may close these issues.

3 participants