fix: collect metrics on scrape, not timeout

[sam-github]

Only the event loop "lag" is still async, see the in-src notes.

Fixes: #180

---

This is a WIP, it lacks tests (though it passes the existing ones).

Open questions:

• should I rearrange the commits? There are some unrelated ones in the PR, I could pull them into a seperate PR set if wanted. I figured out a way to do this that doesn't change the lib/metrics interface for the defaults, for example, so the metrics changes are mostly cleanup and could be done with or without the removal of the timeout.
• are the lib/metrics/ used directly by anyone? I'd like to remove the do-nothing closures for many of them that are called on every collect but are a no-op.
• do we need to support reconfiguration of the default metrics on the same registry? I can do it, its just a bit more state that needs to be removed now, and some notes in source suggested it wasn't wanted. Then again, not doing it might break some users. Opinions?

Also, my typescript is close to non-existent, pay close attention to my attempt to update the typescript definitions.

[SimenB]

This is wonderful, thanks for working on it! I just went to bed, so I'll review properly tomorrow 🙂

[sam-github]

No hurry, enjoy your weekend. I'm willing to remove code for sure, I'll get rid of the timeout relics.

[SimenB]

are the lib/metrics/ used directly by anyone?

it's not supported, so I'm not afraid of breaking people's usage if it. And since next release is a major, I think it's fine.

do we need to support reconfiguration of the default metrics on the same registry?

I don't think so - what's the use case for it?

[SimenB]

I think this looks great! I'm all for landing as is, then going over and remove everything that is deprecated (whether we print a warning or not)

[sam-github]

@sam-github:

do we need to support reconfiguration of the default metrics on the same registry?

@SimenB:

I don't think so - what's the use case for it?

No idea, but existing code supports it:

• prom-client/lib/defaultMetrics.js

  Line 36 in 9b4ef10

  // We might want to consider not supporting running the default metrics function more than once

• prom-client/lib/defaultMetrics.js

  Lines 61 to 66 in 9b4ef10

  	if (!init) {
  	defaultMetric.metricNames.map(
  	globalRegistry.removeSingleMetric,
  	globalRegistry
  	);
  	}

I can reproduce the master behaviour in this PR, but if the comment is correct, then its OK to just delete the support. I'm happy to remove, but I don't know why its there, or if anyone uses it.

[SimenB]

It seems I added that comment 3 years ago - I have no recollection of it 😅

But no, I think it makes sense to throw if called again. IIRC it should do so if we just remove the guard, as that's the default registry behavior

[siimon]

are the lib/metrics/ used directly by anyone?

it's not supported, so I'm not afraid of breaking people's usage if it. And since next release is a major, I think it's fine.

I agree, in my world the public interface to this library is what is in /index.js

[siimon]

It seems I added that comment 3 years ago - I have no recollection of it 😅

But no, I think it makes sense to throw if called again. IIRC it should do so if we just remove the guard, as that's the default registry behavior

I agree here aswell. The only potential problem I could see is in tests where you run it multiple times. But that should probably be handled when you write the tests with a proper cleanup.

[siimon]

I think it looks great, really appreciate your time working on this!

[sam-github]

The only potential problem I could see is in tests where you run it multiple times. But that should probably be handled when you write the tests with a proper cleanup.

Or write the test to use a non-default registry, the throw that is in there will trigger if the default metrics are added multiple times to the same registry. They can be added to a number of different registries.

[sam-github]

Added a test for #329 (comment)

[sam-github]

I addressed the comments, I guess its ready for review again.

It does contain #330, maybe that should land first, then I'll rebase this. Or this could land and close #330. Whatever you all prefer.

[siimon]

@sam-github the changes LGTM! Just got a conflict after the latest merge, so if you could fix that and update the PR and I'll merge right away!

[SimenB]

This is great! 🎉

[sam-github]

@SimenB Thanks for the careful review, I made all the suggested changes, PTAL.