Fix the awaitable collect() calls.

[farcaller]

In case if non-native promises are used, the Promise object won't be instanceof Promise. That's still valid in JS, as Promise is something that implements then(). It's always safe to cast a maybe-promise into native Promise with Promise.resolve().

[zbjornson]

Is anyone actually using non-native promises now? They've been available in Node since Feb 2015 and this library requires native Promise support. I'm kind of inclined to say we shouldn't make this change and should instead tell users implementing custom collect() functions to return a native Promise.

[zbjornson]

The original code was intended to avoid entering the microtask queue unless necessary, for performance (i.e. don't await unless necessary).

What about duck-typing for a thenable?

[farcaller]

That's an interesting point I didn't consider. I suppose duck-typing is fair. Updated the PR.

[farcaller]

Is anyone actually using non-native promises now?

Funnily, yes. ClojureScript's Promesa uses non-native Promises (and that's how I ended up going down this rabbit hole).

[zbjornson]

What do you think about doing this on the consumer side though? i.e. in your custom collect() function, return Promise.resolve(/*the Promesa promise */).

[farcaller]

That's what I'm doing right now. I think it's just a bit of an unexpected behavior from the library to say it takes a promise, but then actually taking a very specific-exactly-JSPromise promise.

I guess, it holds true for the 95% promises flying around, though? Maybe it's worthwhile to mention it in the docs then, to not incur the runtime costs.

[farcaller]

I'm giving this another thought and I wonder: is this actually the use case where the code must be extremely performant? I'd expect that metrics page would be scraped once in a while, or pushed once in a while, where "a while" is dozens of seconds. At that scale, optimizing for microtask scheduling seems a bit futile, is it not?

[zbjornson]

is this actually the use case where the code must be extremely performant?

prom-client gets a surprising number of PRs to improve its performance. I think it's valid to want observability frameworks to be as low of overhead as possible. If you have 100s of metrics with collect() functions, that could have an impact.

I'm +1 on a doc-only change that says the return type needs to be a native Promise. The typescript types are already correct fwiw: they say Promise and not PromiseLike.

[jdmarshall]

@farcaller This is a programming language with a single thread and a high barrier to offloading data processing to other threads.

Every time the collection interval hits, your p95 times will spike. And if you use cluster mode, it will spike on all threads on the same server at exactly the same time, which will definitely hit your p95 times if you have less than 50 servers in your cluster.

There's also GC pressure, which unfortunately the project doesn't have a convenient way to benchmark. Several of the changes I have made are mostly straight-line performance neutral but definitely reduce memory footprint, which I know from similar efforts on a project at work, where I was able to run an additional instance of our app on each machine in the cluster due to reduced heap size, just from using Map instead of {} for the exact same data.

If there was a library like benchmark-regression that was actively maintained, I think it would be worth tracking memory intensity as well.

[jdmarshall]

I noticed this code earlier today when working on #683

I have a different proposal here.

Sometime around Node 16 or 18 (my memory is fuzzy and I no longer work for the company where I could check commit history to verify), an optimization to v8 allowed awaiting of non-promises to be more efficient.

It changed await("foo") to eliminate triggering nextTick(), which I discovered when a couple of tests started throwing bizarre error messages that did not make sense. It turned out that we were mocking an async function with a sync mock, and then part of the setup code started running after the test condition instead of before as it previously had - because v8 was no longer yielding to look at the promise resolution queue if the await was on a synchronous call.

tl;dr: has anyone tried just non-conditionally awaiting the results and benchmarking how much the JIT optimizes that code?

The internet informs me that Bluebird promises at least are compatible with await.