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

feat(prometheus-client): add metric label about root on using PrometheusClientLayer #4907

Merged
merged 8 commits into from
Jul 18, 2024

Conversation

flaneur2020
Copy link
Contributor

@flaneur2020 flaneur2020 commented Jul 17, 2024

Which issue does this PR close?

Closes #4909.

Rationale for this change

I did not found the bucket value in the accessor, but i found a parameter called root might be helpful on this case. To make this PR work as expected, I need to make sure that the bucket should be the prefix of the root parameter.

What changes are included in this PR?

  • refactor: seperate registering metrics and helpers to update metrics with the common labels
  • feat: add root label to all the labels

Are there any user-facing changes?

this PR adds a new label on the prometheus metrics, if you had an existed dashboard, this change may make additional data lines on your chart. however it would not change your chart if you use sum(xx) by (op) or other metric aggregation operations.

to make this change possible, we had to make the type of labels from (&'static str, &'static str) to (&'static str, String), this may degrade a bit performance.

@flaneur2020 flaneur2020 marked this pull request as ready for review July 17, 2024 10:29
@flaneur2020 flaneur2020 requested a review from Xuanwo as a code owner July 17, 2024 10:29
@flaneur2020
Copy link
Contributor Author

a question I need to confirm about this PR: does this root parameter contain the bucket in it?

@Xuanwo
Copy link
Member

Xuanwo commented Jul 17, 2024

does this root parameter contain the bucket in it?

No. root is the base path for all opendal operations. You might want to include name in the metrics.

@flaneur2020
Copy link
Contributor Author

does this root parameter contain the bucket in it?

No. root is the base path for all opendal operations. You might want to include name in the metrics.

thank you for the suggestion, i've added a namespace label (i feel afraid that name might has some risks conflicting with some built-in prometheus labels) in the appended commit. the root label might still be useful for some other cases, i did not remove it after namespace added.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Others LGTM, thanks!

core/src/layers/prometheus_client.rs Outdated Show resolved Hide resolved
@flaneur2020 flaneur2020 force-pushed the add-prometheus-label-about-bucket branch from 45eccd4 to 82e050d Compare July 17, 2024 11:15
@flaneur2020 flaneur2020 requested a review from Xuanwo July 17, 2024 11:19

fn increment_errors_total(&self, scheme: Scheme, op: &'static str, err: ErrorKind) {
fn increment_errors_total(&self, op: &'static str, err: ErrorKind) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking of implementing EncodeLabelSet directly for our lables to avoid to_string() here.

ref: https://docs.rs/prometheus-client/latest/prometheus_client/metrics/family/struct.Family.html#family-with-custom-type-for-performance-andor-type-safety

Would you like to start a new PR for this or just in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me have a try in this pr

Copy link
Contributor Author

@flaneur2020 flaneur2020 Jul 18, 2024

Choose a reason for hiding this comment

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

i switched to a Cow<'static, str> in daab9db which helps reduces copy but can not reduce all the copies.

it turns out that we can not simply add lifetime annotations (for the labels like root and namespace which stored in a singleton struct) in the labels type within promtheus-client, because each metric (with labels together) will be registered into a global registry, which requires the metrics to be 'static.

(imho it would be better to allow us make the label set as a trait object in a metric type, which help us decoupling the lifetime and metrics registration, however it's not allowed in prometheus-client yet)

Copy link
Member

@Xuanwo Xuanwo Jul 18, 2024

Choose a reason for hiding this comment

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

Hi, I'm thinking about making OperationLabels a seperate struct and implement EncodeLabelSet for it. For example:

struct OperationLabels {
  scheme: Scheme,
  op: &'static str,
  namespace: Arc<String>,
  root: Arc<String>,
}

impl EncodeLabelSet for OperationLabels { .. }

In this way, we can constrct OperationLabels and use:

family.get_or_create(&OperationLabels { ... }).inc();

This design will also avoid unexpected breaking changes like field doesn't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, i've changed them to structs, PTAL

@flaneur2020 flaneur2020 force-pushed the add-prometheus-label-about-bucket branch from daab9db to 6dee3e6 Compare July 18, 2024 04:09
@flaneur2020 flaneur2020 requested a review from Xuanwo July 18, 2024 05:04
@flaneur2020 flaneur2020 force-pushed the add-prometheus-label-about-bucket branch from f021eb5 to 23d0404 Compare July 18, 2024 05:07
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit e42a9eb into apache:main Jul 18, 2024
218 of 219 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new feature: add a label about the object store bucket in metric labels
2 participants