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(prometheus-exporter): add unref function check to improve compat… #4613

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

Conversation

acamposruiz
Copy link

Which problem is this PR solving?

This PR introduces a conditional unref feature to the Prometheus Exporter within the OpenTelemetry JavaScript project. Previously, the unref method was called unconditionally, which could lead to potential issues in environments where unref is not available or not needed, thus keeping the process open unnecessarily. This change enhances compatibility and reliability, especially in diverse runtime environments.

Fixes # #4488

Short description of the changes

  • Added a check to ensure unref is only called if it is a function, preventing potential runtime errors in environments where unref might not be available.
  • This ensures the HTTP server created by the Prometheus exporter does not keep the process open when not required, allowing for cleaner shutdowns and resource management.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

The changes were tested by:

  • Existing tests pass

Checklist:

  • Followed the style guidelines of this project

@acamposruiz acamposruiz requested a review from a team as a code owner April 8, 2024 10:14
Copy link

linux-foundation-easycla bot commented Apr 8, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@pichlermarc
Copy link
Member

@acamposruiz thanks for opening this PR. I don't have a bun testing setup at the moment, but I've seen a PR (oven-sh/bun#8675) that added unref support to the server.

Would you mind testing with a more recent version? Maybe this change is not needed anymore?

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

Successfully merging this pull request may close these issues.

None yet

2 participants