I don't think this comment is valid anymore, please remove it.

Style: cleaner to put the first arg in a new line. Please use the same style in other places too.

Style:

• Would be good to include the invalid flushPeriod in the error message
• Looks like the other validation uses this to, could use DruidException for newer ones. Something like:

throw DruidException.forPersona(DruidException.Persona.OPERATOR)
                          .ofCategory(DruidException.Category.INVALID_INPUT)
                          .build(
                                  "Invalid value for 'druid.emitter.prometheus.flushPeriod'[%s] specified, flushPeriod must be > 0 if specified.",
                                  waitForShutdownDelay
                              )
                          );

Thanks for the docs! Minor suggestion:

Also, do you have guidance on what an operator should set this value to for TTL eviction (perhaps something as a function of emission period)?

Yeah, good point. I guess it would have to account for the scraping frequency too.
So, a good value would be something like 3 * scrape period or 3 * emission period, whichever is larger.

I think this comment is stale?

Extract entry.getValue() to a variable and reuse since it's used a few times in this loop :)

DimensionsAndCollector could accept the flush period directly, initialized from the Prometheus config that’s plumbed through Metrics. Then this code could become if (entry.getValue().isExpired()) {} where this fixed config isn't redundantly passed in

Is this clear() operation on SimpleCollector thread-safe? Doing a quick scan of the docs suggest that the client libraries must be thread-safe, but wanted to confirm that clearing can concurrently happen with the metric updates in emitMetric()

Side note: I think it would be better to move some of these methods inside DimensionsAndCollector to avoid abstraction leaks. But that refactor can be done later since it'd also require moving the metric updates from emitMetric() into this class.

Yup, confirmed that the Map getting cleared is a ConcurrentHashMap

nit: can be simplified to updateTimer.restart();

It looks like clear() removes all children for the metric. Do we instead want to use remove() to target a specific time series by specifying the corresponding set of label values?

Without that, metrics that track things like task ID or query ID, for example, task/run/time or query/wait/time mentioned in #14638 wouldn’t get evicted if they’re being updated frequently.

When we call metric.resetLastUpdateTime(), should we pass the appropriate label set and track it? Then in cleanUpStaleMetrics(), we could check the label sets and clear only the stale time series rather than clearing all of them or none at all. What do you think?

Hmm, I suppose removing the children for specific labels would be a more complete solution.
But it could blow up the memory usage pretty fast since we would now be tracking the last updated time for every unique set of label values for every metric.

I think the issue this PR is trying to address is that if a certain metric stops being reported by a service
altogether (irrespective of label values), it should not remain in the collector anymore. e.g. in leadership changes.

Also, the staleness concern should really only apply to gauge metrics. So if timer (histogram) metrics like task/run/time and query/wait/time are still being updated by the current service (for any label), we should continue emitting them. Getting rid of the expired labels becomes more of a mem usage concern than anything else.

But with gauge metrics like say segment/size (which represents bytes of segment available on a historical for a given datasource), I agree that the story is different.
If all segments of a datasource are removed from a historical, we should definitely stop reporting segment/size for that datasource.

But I would suggest we add a comment and postpone the full solution for gauge metrics for a later PR.
Ideally, we would want to write up an embedded test (using Prometheus testcontainer) and verify the behaviour of gauge metrics in such cases and then proceed with a solution.

What are your thoughts, @abhishekrb19 , @aho135 ?

Hmm, I suppose removing the children for specific labels would be a more complete solution.
But it could blow up the memory usage pretty fast since we would now be tracking the last updated time for every unique set of label values for every metric.

Yeah, good callout on the memory usage when tracking every unique combination of label sets per metric. Prometheus isn’t designed for very high cardinality labels, and the Prometheus SimpleCollector in the java library already tracks the unique label sets (children) internally. On the extension side, we’d only be dealing with a subset of those children, since we’d remove the ephemeral “stale” entries anyway (if there's a way to leverage the collector’s internal state somehow, that would be even better). So I’m not too concerned about the additional memory this would require, though it might still be worth validating in a cluster with some labels with reasonable cardinality and load.

I’m also okay with the suggestion to start without labels for now and revisit this (or another solution) once we have a better understanding of the additional memory overhead it might introduce. The current implementation should remain extensible enough to support label tracking in the future if needed. We can keep #14638
open and call out in the documentation that the TTL expiration only applies to certain metric types and not to individual timeseries'.
@aho135 @kfaraz what do you think?

Yeah, I was also concerned about memory usage. Especially where folks are adding queryId as a dimension like in #14638.

Sounds good to me to tackle the label problem in a separate PR. We can say that this PR specifically targets resetting the metric value in the leader election scenario

Could you add the same metric with different label values so that it actrs as a different timeseries and then verify that one exists while the other one has expired?

I guess we can defer this test since we're not considering labels when clearing in this PR

nit: you could assign Duration.standardSeconds(ttlSeconds) in the constructor itself

I think this isn't needed?

I was just copying the current format. Ex: https://github.com/apache/druid/blob/master/website/.spelling#L2455

I can remove this if it's not needed though

An additional note on the behavior for clarity:

Nit: This can be a Duration to avoid repeated created of a Duration object in isExpired.

Nit: we could pass the entire config object to the constructor rather than these 6 args.

Nit: seems like this part can be commoned out.

This condition should never trigger in production. if it does, it’s a bug, so we could throw DruidException.defensive() to indicate invalid usage for developers. Given it’s configured correctly now, this should be fine and we can update this later.