Skip to content

make metrics collection independed from metrics creation #60

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

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

avolokhov
Copy link
Contributor

This PR allows metrics collection to be concurrent to metrics creation.
SwiftPrometheus' metrics types are already thread-safe, there's no need in holding a global lock while calling .collect on every single metric.

Checklist

  • [ + ] The provided tests still run.
  • [ N/A ] I've created new tests where needed.
  • [ N/A ] I've updated the documentation if necessary.

Motivation and Context

Currently metrics collection is done under a global lock and it's stopping new metrics from being created in parallel. Allowing this to happen in parallel will reduce the impact metrics framework is having on the running system.

Description

PrometheusClient.collect now only holds global lock to get a list on metrics in a thread safe manner. metric.collect on individual metric is called without a global lock and relying on corresponding metric type to be thread safe.

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Good catch, thanks Anton!

@ktoso ktoso added this to the V 1.0.0 Alpha 15 milestone Aug 23, 2021
Copy link
Collaborator

@MrLotU MrLotU left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR

@MrLotU MrLotU merged commit 196573e into swift-server:master Aug 24, 2021
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.

3 participants