Skip to content

refactor(layers/prometheus-client): provide builder APIs#5073

Merged
Xuanwo merged 14 commits intoapache:mainfrom
koushiro-contrib:prometheus-client-consistent-apis
Sep 3, 2024
Merged

refactor(layers/prometheus-client): provide builder APIs#5073
Xuanwo merged 14 commits intoapache:mainfrom
koushiro-contrib:prometheus-client-consistent-apis

Conversation

@koushiro
Copy link
Copy Markdown
Member

@koushiro koushiro commented Aug 30, 2024

Which issue does this PR close?

Related issue: #5069 (comment)
Related PR: #5072

Rationale for this change

Provide similar APIs for PrometheusLayer and PrometheusClientLayer

What changes are included in this PR?

  • support configurable params like PrometheusLayer
  • add PrometheusClientLayer::builder
  • add PrometheusClientLayerBuilder
  • fix metrics name
  • remove OperationErrorsTotalLabels
  • add more examples in doc

Are there any user-facing changes?

API breaking change

@github-actions github-actions Bot added the releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" label Aug 30, 2024
@koushiro
Copy link
Copy Markdown
Member Author

@Xuanwo maybe you can also take a look at this PR?

@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Aug 30, 2024

@Xuanwo maybe you can also take a look at this PR?

This PR appears to include many changes from #5072. Perhaps we should merge #5072 first?

@koushiro
Copy link
Copy Markdown
Member Author

@Xuanwo maybe you can also take a look at this PR?

This PR appears to include many changes from #5072. Perhaps we should merge #5072 first?

Yeah, I just want to give you more information about why I wrote this to provide similar APIs.

Comment thread core/src/layers/prometheus_client.rs Outdated

fn register(&self, registry: &mut Registry) {
registry.register_with_unit(
"opendal_operation_duration",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we can't use observe::METRIC_OPERATION_DURATION_SECONDS.name() here, prometheus-client will generate opendal_operation_duration_seconds_seconds metric

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

maybe we should use register instead of register_with_unit here, then we could use observe::METRIC_OPERATION_DURATION_SECONDS.name()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see. Let's change it.

Comment thread core/src/layers/prometheus_client.rs
@koushiro koushiro changed the title refactor(layers/prometheus-client): provide consistent APIs refactor(layers/prometheus-client): provide builder APIs Sep 3, 2024
@koushiro koushiro marked this pull request as ready for review September 3, 2024 07:19
@koushiro koushiro requested a review from Xuanwo September 3, 2024 12:51
Copy link
Copy Markdown
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 84e8290 into apache:main Sep 3, 2024
@koushiro koushiro deleted the prometheus-client-consistent-apis branch September 3, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants