apache / opendal

Apache OpenDAL: One Layer, All Storage.
https://opendal.apache.org
Apache License 2.0
3.45k stars 482 forks source link

refactor(layers/prometheus): provide builder APIs #5072

Closed koushiro closed 2 months ago

koushiro commented 2 months ago

Which issue does this PR close?

Related issue: https://github.com/apache/opendal/pull/5069#discussion_r1736066873 Related PR: #5073

Rationale for this change

Provide similar APIs for PrometheusLayer and PrometheusClientLayer

What changes are included in this PR?

Are there any user-facing changes?

API breaking change

koushiro commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.