1. Would it make sense to define and maintain a set of Micrometer metrics and increment them here?
2. Would it make sense to fire a PolarisEvent here?

(I don't really know what an IRC server is supposed to do with these scan or commit metrics 😅)

I'm not sure what those are 😅 my intentions were to have the default behaviour remain the same, and adding that may change the default behaviour

😄 - no worries, we can look into that later.

My recommendation was to store them in persistence, so that later we can serve them later, I recommend the same to @pingtimeout https://lists.apache.org/thread/koogjvb73hbl9ospmm7owsdx8oh67wk1

I guess we would need to purge that table frequently. Not sure if it's a good default to store in persistence without providing the purge functionality. Could that be an alternate CDI bean?

Agree, though It can be any persistence, not necessarily an OLTP imho an OLAP is a valid consideration too, so is dumping in object storge so analytical engines can make a table on top of from Polaris POV, it seems to me that we are settling on catalog name and tble identifiers in addition to report from interface POV, why is realm-id not part of it is my doubt, why is authenticate prinicipals not part of these metric ?

config.type() is fixed in runtime... Can this bean be @ApplicationScoped?

This producer is annotated @ApplicationScoped but it returns a PolarisMetricsReporter, which is @RequestScoped. That’s problematic in CDI because an @ApplicationScoped bean lives for the entire application lifecycle, while a @RequestScoped bean only lives for a single request.
If the reporter (or a future implementation) ever injects request-scoped beans like RealmContext or SecurityContext, this setup would either fail at deployment or produce invalid references once the request ends.

Instead of producing a reporter directly in an app-scoped producer, introduce a small factory:

@ApplicationScoped
public class PolarisMetricsReporterFactory {

  public PolarisMetricsReporter getOrCreate(String type) {

  }
}

Similar to MetaStoreManagerFactory, which ensure the scope correct, while avoid creating a new object for every request. It is also more flexible when we introduce more reporter types.

A factory is ApplicationScoped(or Singleton), but a bean can be ApplicationScoped or RequestScoped:

• Should the bean exist only during the lifespan of a request? If so, use @RequestScoped.
• Should the bean exist during the lifespan of the application and should the state of the bean be shared between all the requests? If so, use @ApplicationScoped.

So, to use ApplicationScoped here, a factory is probably clearer.

I'm -1 on the factory approach, The factory approach in Polaris is generally used for creating and caching realm-scoped beans. The producer method approach OTOH is used for selecting beans at runtime according to their @Identifier.

Here, it's clearly an application-scoped bean, selectable at runtime with its @Identifier.

The correct fix for me is just to replace @RequestScoped with @ApplicationScoped in the producer method:

@ApplicationScoped
public MetricsReporter metricsReporter(...)

The correct fix for me is just to replace @RequestScoped with @ApplicationScoped in the producer method:

+1

@cccs-cat001 please change the scope of the DefaultMetricsReporter bean to also @ApplicationScoped – there is no reason for it to be request-scoped.

Once this change is done we should be good 🤞

Actually, given this producer method, I do not think DefaultMetricsReporter needs a scope annotation at all. It is always created via this method.

Okay I see, I think you're right and we don't need the annotation. Other examples with @ApplicationScope in the ServiceProducer follow the same pattern.

Actually, given this producer method, I do not think DefaultMetricsReporter needs a scope annotation at all. It is always created via this method.

Yes and no :-) It doesn't need the annotation but 1) all other beans in the same situation are annotated and 2) annotating the bean allows for direct injection, e.g. in a test:

@Inject @Identifier("default") MetricsReporter defaultMetricsReporter;

So, I'm rather in favor of anotating both places, even if I understand that this can cause some confusion.

Annotating both places SGTM 👍 Using @ApplicationScoped, right?

Can we remove this scope annotation if this is meant to be application scoped? More details are here, https://github.com/apache/polaris/pull/2887/files#r2487347077

Makes sense! Good catch!

Not a blocker: would the name IcebergMetricsReporter be more descriptive as this is only for iceberg tables, other tables managed by Polaris won't be able to report the metrics?

Good point. But let's merge this PR (the author has been kind enough to address our collective feedback for two weeks now) and address this one in a follow-up task.

there was another TODO mentioned here #2887 (comment)

If my time allows I'd be happy to help contribute that, and if we can get the requirements defined beforehand :)