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

Add download rate metrics for buildfarm:worker #1418

Merged
merged 5 commits into from
Aug 11, 2023

Conversation

amishra-u
Copy link
Contributor

Download rate metrics for worker was missing. Added metrics for worker download rate.

@amishra-u amishra-u marked this pull request as ready for review August 8, 2023 02:37
@amishra-u amishra-u requested a review from werkt as a code owner August 8, 2023 02:37
Copy link
Member

@werkt werkt left a comment

Choose a reason for hiding this comment

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

approved pending nit fix

@@ -68,6 +69,9 @@

@Log
public class ShardWorkerInstance extends AbstractServerInstance {
private static final Counter ioMetric =
Copy link
Member

Choose a reason for hiding this comment

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

Not static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blindly copied from ShardInstance :( removed

Copy link
Contributor Author

@amishra-u amishra-u Aug 8, 2023

Choose a reason for hiding this comment

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

Counter's register function appends (registers) metrics to a static instance of the CollectorRegistry.

After I modified the ioMetric variable to be non-static, the unit tests in ShardWorkerInstanceTest began to fail. This is because each test generates a fresh ShardWorkerInstance instance, and with every instance creation, it tries to register the same metrics with the CollectorRegistry again and fails with already register error.

Although buildfarm:worker at runtime only creates single instance of ShardWorkerInstance and even making the ioMetric non-static will work but it is not configured to be singleton. Also ioMetric need to be registered only once not per instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@werkt Can you please merge it.

Copy link
Member

Choose a reason for hiding this comment

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

Alright. We will need to get rid of the statics overall elsewhere.

@werkt werkt merged commit 18ae72e into buildfarm:main Aug 11, 2023
@amishra-u amishra-u deleted the worker_read_metrics branch August 18, 2023 18:06
@amishra-u amishra-u restored the worker_read_metrics branch August 18, 2023 18:06
@amishra-u amishra-u deleted the worker_read_metrics branch August 18, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants