fix(@opentelemetry/exporter-prometheus): unref prometheus server to prevent process running indefinitely

[mothershipper]

Which problem is this PR solving?

Prometheus exporter is holding the process open for scripts that are meant to exit.

Short description of the changes

Unrefs the prometheus exporter to allow the process to exit.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Run in our production environment, with a server that was purposely failing to boot -- something like:

const main = async () => {
  await initOtel()
  await initServer() // throws
}

main.catch(err => console.log)

Previously, prometheus would hold the process open. With this change, the process exits as expected.

[dyladan]

Good catch

[dyladan]

If its easy a test would be nice. If not, then this is such a small change its unlikely to be a problem.

[codecov]

Codecov Report

Merging #2558 (95d36da) into main (b531acf) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 95d36da differs from pull request most recent head 4fc5b3b. Consider uploading reports for the commit 4fc5b3b to get more accurate results

@@            Coverage Diff             @@
##             main    #2558      +/-   ##
==========================================
+ Coverage   93.05%   93.08%   +0.02%     
==========================================
  Files         140       58      -82     
  Lines        5172     1894    -3278     
  Branches     1111      408     -703     
==========================================
- Hits         4813     1763    -3050     
+ Misses        359      131     -228     

Impacted Files	Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts	0.00% <0.00%> (-100.00%)	⬇️
...opentelemetry-core/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...pentelemetry-core/src/platform/node/performance.ts	0.00% <0.00%> (-100.00%)	⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	84.48% <0.00%> (-15.52%)	⬇️
...ntelemetry-sdk-metrics-base/src/BoundInstrument.ts
...sync-hooks/src/AbstractAsyncHooksContextManager.ts
...k-metrics-base/src/export/ConsoleMetricExporter.ts
...telemetry-sdk-metrics-base/src/export/Processor.ts
...pentelemetry-sdk-metrics-base/src/BatchObserver.ts
... and 77 more

[mothershipper]

We've got this monkeypatched it in our environment, so I'm not in a rush to get this in upstream -- I can take a look at adding tests after hours.

Would you mock createServer and just test for unref being called? Usually I prefer the behavioral tests, but I'm not sure offhand how I'd safely test for the reference count not increasing / exit isn't hung.

[dyladan]

I can't see anywhere in the docs to check if a socket is unref'd so I guess the only way to do it would be with a mock

[mothershipper]

took a stab at the test

[dyladan]

This branch is out of date and I don't have permission to update your branch so I can't merge it. Please update the branch and if you give maintainers permission to push to the branch that makes it much easier for us.

[mothershipper]

@dyladan I've updated, but to be honest, I'm not entirely sure how to give you access -- looking into that now. Would you consider adding a link to those instructions to the CONTRIBUTING.md doc if I can find some?

[dyladan]

Yeah it should be a checkbox when you create the PR that says something along the lines of "allow maintainers to push changes to the branch"

Not sure how its done after the fact but I know its possible.

[mothershipper]

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

^suggests it's possible, but I don't see that option at all 😕

[mothershipper]

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

^suggests it's possible, but I don't see that option at all 😕

[mothershipper]

I can recreate it if that's going to be easier

[dyladan]

It should be right underneath the screenshot you posted

Don't bother recreating. The tests are failing btw

[Flarna]

Yeah it should be a checkbox when you create the PR that says something along the lines of "allow maintainers to push changes to the branch"

As far as I know this works only for personal forks but not for forks owned by an organisation.

[dyladan]

Oh I didn't see it was an org fork. Tests are still failing.

[mothershipper]

I agree - it's just not there on my end.

Apologies on the test, pushing a fix now - shouldn't have ran mine in isolation, forgot to unstub the http lib.

[mothershipper]

Ah, good to know! I can open another PR to add instructions / caveats to the CONTRIBUTING.md

[mothershipper]

tests should be passing now, ran the full suite locally this time