Core: don't override LoggingMetricsReporter

[mst]

the current metrics reporter is overwritten if it's a LoggingMetricsReporter. If it's not LoggingMetricsReporter, metrics reporters are combined.

Imho, the only reason to do this is to prevent multiple LoggingMetricsReporters. However, MetricsReporters::combine takes care of this case already as LoggingMetricsReporter is a singleton and the combine method uses an IdentityHasMap.

[gaborkaszab]

Hi @mst ,
I was looking at this code myself recently too. Initially I also thought that we shouldn't swap LoggingMetricsReporter here and we should combine it with the one received as a param. However, in that case there is no way to not use LoggingMetricsReporter in case you, for some reason, don't want logging for the metrics. So I found that the code as it is now kind of makes sense in a way.
In case you want to provide a MetricsReporter but also want to keep the default LoggingMetricsReporter, you can still provide a combined reporter to this function.

This is what I did in my experiment.

I'm curios what @nastra think about this.

[nastra]

Hi @mst , I was looking at this code myself recently too. Initially I also thought that we shouldn't swap LoggingMetricsReporter here and we should combine it with the one received as a param. However, in that case there is no way to not use LoggingMetricsReporter in case you, for some reason, don't want logging for the metrics. So I found that the code as it is now kind of makes sense in a way. In case you want to provide a MetricsReporter but also want to keep the default LoggingMetricsReporter, you can still provide a combined reporter to this function.

This is what I did in my experiment.

I'm curios what @nastra think about this.

Thanks for the summary @gaborkaszab, that is in fact exactly the intention here. For example, Trino uses a noop reporter for testing and doesn't want things to be logged. So I don't think we should be changing anything here.
Thanks for looking into this @mst but I'll go ahead and close this PR