refactor(layers/prometheus): provide builder APIs

[koushiro]

Which issue does this PR close?

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

Rationale for this change

Provide similar APIs for PrometheusLayer and PrometheusClientLayer

What changes are included in this PR?

• add PrometheusLayer::builder
• add PrometheusLayerBuilder
• impl Default for PrometheusLayer
• move path_label_value into observe mod
• add more examples in doc

Are there any user-facing changes?

API breaking change

[Xuanwo]

We don't need this API. PrometheusLayer::new() should be enough.

[koushiro]

IMO, PrometheusLayer::new should just be a helper function that uses the default config and registers the metrics with the default config to the registry.

[Xuanwo]

Personally, I don't believe we should perform register during new unless the upstream API forbid it. Creating the layer itself should have no side effects. Everything should occur during the layer.

So PrometheusLayer::new() should just take a registry and return Self. We will perform the registry during Layer::layer.

[koushiro]

So how about PrometheusClientLayer in #5073

[Xuanwo]

Okay, how about this: Let's use your previous proposed builder's API.

let layer: impl Layer = PrometheusClientLayer::builder()
    .operation_bytes_buckets(vec![1.0, 2.0])
    .register(registry);

PrometheusClientLayer::builder() returns a PrometheusClientLayerBuilder. Users can only use this layer after calling register. We can provide default() if the upstream lib has default registry concept.

What do you think?

[koushiro]

So do we need to use similar APIs in PrometheusLayer?

[Xuanwo]

So do we need to use similar APIs in PrometheusLayer?

I believe this design can adapt to all metrics libraries. What do you think?

[koushiro]

Yes, if you agree with the design, I'll apply it to PrometheusLayer and PrometheusClientLayer.
But I'm not sure if this design will fit well with other metric layers, I'm not very optimistic after using the metrics library.

[Xuanwo]

I feel like we can call with_recorder direcly: https://docs.rs/metrics/latest/src/metrics/recorder/mod.rs.html#126

[Xuanwo]

Thanks a lot! This API design is really clean and easy to use. There are some nits on the public API.

[Xuanwo]

How about returning an error instead of just unwrapping?

[koushiro]

Should we return prometheus::Error or opendal::Error?

[Xuanwo]

opendal only returns opendal::Error, we can wrap it inside.

[koushiro]

There are some weird errors in CI

error[E0282]: type annotations needed
   --> src/services/icloud/core.rs:530:25
    |
530 |             Some(it) => Ok(Some(it.drivewsid.clone())),
    |                         ^^ cannot infer type of the type parameter `E` declared on the enum `Result`
    |
help: consider specifying the generic arguments
    |
530 |             Some(it) => Ok::<std::option::Option<std::string::String>, E>(Some(it.drivewsid.clone())),
    |                           +++++++++++++++++++++++++++++++++++++++++++++++

error[E0283]: type annotations needed
   --> src/services/icloud/core.rs:530:25
    |
530 |             Some(it) => Ok(Some(it.drivewsid.clone())),
    |                         ^^ cannot infer type of the type parameter `E` declared on the enum `Result`
531 |             None => Ok(None),
532 |         }?;
    |          - type must be known at this point
    |
note: multiple `impl`s satisfying `types::error::Error: std::convert::From<_>` found
   --> src/types/error.rs:418:1
    |
418 | impl From<prometheus::Error> for Error {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: and another `impl` found in the `core` crate: `impl<T> std::convert::From<T> for T;`
    = note: required for `std::result::Result<std::option::Option<std::string::String>, types::error::Error>` to implement `std::ops::FromResidual<std::result::Result<std::convert::Infallible, _>>`
help: consider specifying the generic arguments
    |
530 |             Some(it) => Ok::<std::option::Option<std::string::String>, E>(Some(it.drivewsid.clone())),
    |                           +++++++++++++++++++++++++++++++++++++++++++++++

But if I change the icloud code, these errors are gone

-        let id = match node.items.iter().find(|it| it.name == name) {
-            Some(it) => Ok(Some(it.drivewsid.clone())),
-            None => Ok(None),
-        }?;
-        Ok(id)
+       Ok(node
+          .items
+          .iter()
+          .find(|it| it.name == name)
+          .map(|it| it.drivewsid.clone()))

[Xuanwo]

But if I change the icloud code, these errors are gone

I think it's a new Clippy lint introduced in the latest Rust version. Would you like to submit a new PR to address it?

[Xuanwo]

9b5c096 (#5072)

I apologize for not providing a clear review. I suggest adding a parse_prometheus_error(err: prometheus::Error) -> Error as a standalone function in the Prometheus layer module itself, rather than within Error.

In this way, we keep our public types clean and easy to maintain.

[Xuanwo]

Thank you very much. We are very close to merging. I found a few places I missed in the previous round.

[Xuanwo]

Perfect, thank you for your patience!