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

refactor(layers/prometheus-client): remove useless scheme field from PrometheusAccessor and PrometheusMetricWrapper type #5026

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

koushiro
Copy link
Contributor

Which issue does this PR close?

No

Rationale for this change

simplify the code

What changes are included in this PR?

  • remove useless scheme field from PrometheusAccessor and PrometheusMetricWrapper type
  • add help for some metrics

Are there any user-facing changes?

No

@koushiro koushiro requested a review from Xuanwo as a code owner August 20, 2024 08:11
@koushiro koushiro changed the title refactor(layers/prometheus-client): remove useless scheme field from PrometheusAccessor and PrometheusMetricWrapper type refactor(layers/prometheus-client): remove useless scheme field from PrometheusAccessor and PrometheusMetricWrapper type Aug 20, 2024
registry.register("opendal_requests", "", requests_total.clone());
registry.register("opendal_errors", "", errors_total.clone());
registry.register(
"opendal_requests",
Copy link
Contributor Author

@koushiro koushiro Aug 20, 2024

Choose a reason for hiding this comment

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

@Xuanwo What do you think of removing the opendal_ prefix of these metrics?
Users can customize prefix through the following code:

let builder = services::Memory::default();
let mut registry = prometheus_client::registry::Registry::default();

let op = Operator::new(builder)
        .expect("must init")
        .layer(PrometheusClientLayer::new(registry.sub_registry_with_prefix("opendal")))
        .finish();

Copy link
Member

Choose a reason for hiding this comment

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

I want to retain the opendal_ prefix to clearly indicate that these metrics are emitted by opendal, similar to how alertmanager does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But for PrometheusLayer (not PrometheusClientLayer), there is no builtin opendal_ prefix in the metrics.

Copy link
Contributor Author

@koushiro koushiro Aug 20, 2024

Choose a reason for hiding this comment

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

Actually, I want the metrics exported by these two layers to be the same, the only difference being the different prometheus libraries they use, but that is not the case now.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to add prefix for PrometheusLayer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will consider unifying the metrics of these two prometheus layers in a later PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I will consider unifying the metrics of these two prometheus layers in a later PR.

Thanks a lot for the effort!

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.

Great, thanks a lot!

@Xuanwo Xuanwo merged commit 79a7d11 into apache:main Aug 20, 2024
223 of 224 checks passed
@koushiro koushiro deleted the remove-useless-scheme branch August 20, 2024 12:52
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.

2 participants