Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: don't override LoggingMetricsReporter #12092

Closed
wants to merge 1 commit into from

Conversation

mst
Copy link

@mst mst commented Jan 24, 2025

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.

@github-actions github-actions bot added the core label Jan 24, 2025
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.
@mst mst force-pushed the keep_logging_metrics_reporter branch from b56e25e to de47938 Compare January 24, 2025 15:45
@mst mst changed the title Spark: don't override LoggingMetricsReporter Core: don't override LoggingMetricsReporter Jan 24, 2025
@gaborkaszab
Copy link
Collaborator

gaborkaszab commented Jan 29, 2025

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
Copy link
Contributor

nastra commented Jan 29, 2025

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

@nastra nastra closed this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants