I worry about running a measurement with a 10s timeout from within Collect(), which typically gets called every 15s or so.

I settled on that number because a) it's sufficiently high as to indicate a serious problem, b) it won't disrupt the default 1m scrape interval (source), and c) even with a low interval like 15s it still will not interfere with scrapes.

We could lower this a bit though, what do you think?

I guess, what's the expectation for how quicklyCollect() returns?

// This method may be called concurrently and must therefore be// implemented in a concurrency safe way. Blocking occurs at the expense// of total performance of rendering all registered metrics. Ideally,// Collector implementations support concurrent readers.

If NOTIFY is hobbled, then we're blockingCollect() for up to 10s, which doesn't seem like we're playing nice.

I think it's acceptable to haveCollect() block while we measure the latency, provided we don't overrun a reasonable scrape interval. In most architectures it's rare to have multiple scrapers scraping the same targets, soCollect()should only ever be called once at a time.

Alternatively we could spawn a background process which measures latency periodically andCollect() simply reads its state.

I like the idea of separating measurement in the background, but in practice you need to be careful you don't introduce goleak test flakes.

The only hesitation I have with background measurement is that scrapes will be showing stale data, which may give operators the wrong impression.

This architecture should be fine with multiple collections, as the unique ping will just get ignored by other calls to Collect().

OK, let's give it a try:a7c042f

I decided to double back to synchronous-only collection:33a2a1d
The complexity background collection introduced wasn't worth the benefit; I've satisfied my curiosity now 😅

having each measurement be an identical message (and ignoring it) could lead to previously timed-out measurements getting processed here. That could lead to very strange results such as a ping arriving beforestart is set, which will result in therecv latency being two millennia 🙀

Good call! OK, I'll refactor to generate a unique message and only mark the latency roundtrip done when I observe that.

Let me know what you think of65f57b1 please

49d2002 supercedes the above

49d2002 supercedes the above

YOLO-ed this a bit because Go's scheduler gives a goroutine at most 20ms on CPU before it is pre-empted; I added a bit of extra time to be sure. I ran this test with-count=100 and didn't observe any flakes, but I'm open to better approaches.

This is probably OK for now, although the scheduler latency on Windows can apparently vary.

The approach I like to use for controlling time is to pass in achan time.Time. Any time something tries to determine the current time then comes from reading that channel.

This feels too brittle to me. Even if you ran it 100 times locally, we've observed really big variances in scheduling on CI.

I think you should simplify andfake the PubSub, not wrap it with a delay. Then you only need to have a single measurer, and you send it a bogus ping prior to sending the real one it published. Added bonus is that is will execute in microseconds not a second.

633365d

This is probably OK for now, although the scheduler latency on Windows can apparently vary.

The approach I like to use for controlling time is to pass in achan time.Time. Any time something tries to determine the current time then comes from reading that channel.

I like the idea of separating measurement in the background, but in practice you need to be careful you don't introduce goleak test flakes.

This feels too brittle to me. Even if you ran it 100 times locally, we've observed really big variances in scheduling on CI.

I think you should simplify andfake the PubSub, not wrap it with a delay. Then you only need to have a single measurer, and you send it a bogus ping prior to sending the real one it published. Added bonus is that is will execute in microseconds not a second.

Small quibble with the name: "interval" sounds to me like a periodic thing, so I would suggestLatencyMeasureTimeout

Good call
Thanks for the patience on this PR!