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

Extract repositories metrics into its own class #103034

Merged

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Dec 6, 2023

This PR is a follow up of
#102505 (comment) that move the repositories metrics management into its own class which is then passed around instead of relying on the raw meterRegistry and string metric names.

This PR is a follow up of
elastic#102505 (comment)
that move the repositories metrics management into its own class which
is then passed around instead of relying on the raw meterRegistry and
string metric names.
@ywangd ywangd added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >refactoring v8.12.0 labels Dec 6, 2023
@ywangd ywangd requested a review from idegtiarenko December 6, 2023 05:17
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Dec 6, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

private final LongHistogram exceptionHistogram;
private final LongHistogram throttleHistogram;

public RepositoriesMetrics(MeterRegistry meterRegistry) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new class. Everything else is cascading changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering its structure is there a value in converting this class to a record and replace constructor with a static factory?
This way we will not need to declare getters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I changed it to be a record.

@ywangd
Copy link
Member Author

ywangd commented Dec 6, 2023

@JVerwolf What is the status of #102435? Do you want me to hold this PR till yours is over? Thanks!

Copy link
Contributor

@idegtiarenko idegtiarenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left optional suggestion, otherwise 👍

@ywangd
Copy link
Member Author

ywangd commented Dec 7, 2023

@elasticmachine run elasticsearch-ci/part-3

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 7, 2023
@elasticsearchmachine elasticsearchmachine merged commit b9c2980 into elastic:main Dec 7, 2023
15 checks passed
@ywangd ywangd deleted the consolidate-repositories-metrics branch December 7, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >refactoring Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants