[SPARK-21195][CORE] MetricSystem should pick up dynamically registered metrics in sources

[robert3005]

What changes were proposed in this pull request?

MetricSystem picks up new metrics from sources that are added throughout execution. If you do measurements via dynamic proxies you might not want to redeclare all metrics that the proxies will create and you'd prefer them to get populated as they're being produced. Right now all sources are processed only onceat startup and metrics are picked up only if they have been registered statically at compile time. Behaviour I am proposing lets you not have to declare metrics in two places.

This had been previously suggested in #18406 and #29980. I have reduced the scope of the change to just dynamic metric registration.

Why are the changes needed?

Currently there's no way to access MetricRegistry that MetricsSystem uses to hold its state and as such it's not possible to reprocess a source. MetricsSystem throws if any metric had already been registered previously.

n.b. the MetricRegistry is added as a constructor argument to make testing easier but could as well be accessed via reflection as a private variable.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added tests

[dongjoon-hyun]

ok to test

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39905/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39905/

[SparkQA]

Test build #135325 has finished for PR 31267 at commit 56706ff.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[robert3005]

Looks like jenkins tests pass fine andn giithub actions flaked

[github-actions]

Test build #752447031 for PR 31267 at commit a951f24.

[github-actions]

Test build #752448650 for PR 31267 at commit c808690.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42000/

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41999/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41999/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42000/

[SparkQA]

Test build #137423 has finished for PR 31267 at commit c808690.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[mridulm]

Thanks for working on this @robert3005, took a quick pass through it.

[mridulm]

+CC @HyukjinKwon, @jerryshao who have worked on this in past.

[HyukjinKwon]

Thanks for cc'ing me @mridulm.

Actually I reviewed this. It makes sense but I am not very used to this code path to be honest ...

[mridulm]

Thanks @HyukjinKwon :-)
+CC @srowen

[SparkQA]

Test build #137635 has finished for PR 31267 at commit c808690.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[robert3005]

This pr shouldn't be stale. I forgot about but will address comments eow

[robert3005]

Addressed the comments. While reviewing the code I noticed there was a small thing that I missed - the error on duplicate metric name will happen now in the listener so I kept the logic that suppressed that error when registering

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46627/

[SparkQA]

Test build #142115 has finished for PR 31267 at commit 2e6f29b.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46686/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46686/

[SparkQA]

Test build #142174 has finished for PR 31267 at commit e375448.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #143664 has finished for PR 31267 at commit e375448.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!