Fix #342: Async support for push_metrics #343
Conversation
There was a problem hiding this comment.
Thanks for both tracing this bug all the way down as well as proposing a bug fix.
I am likely the wrong maintainer to review this pull request as (a) I am not a user of pushing metrics to a Prometheus server and (b) I am of the opinion that pushing metrics is out of scope for this library.
With that in mind I would prefer another maintainer to get involved as well.
|
Fixes #342 Properly linking back to the detailed bug report. |
|
Tagging @koushiro @breeswish for review :) |
adamchalmers
force-pushed
the
master
branch
5 times, most recently
from
e60486e
to
95b848b
Compare
adamchalmers
force-pushed
the
master
branch
from
95b848b
to
fa8d268
Compare
|
@mxinden @breeswish Hi! Just pinging y'all for another review :) |
| handle_push_response(response.status(), push_url) | ||
| } | ||
|
|
||
| async fn push_async<S: BuildHasher>( |
There was a problem hiding this comment.
This part is very similar to fn push. Is it possible to use something like https://docs.rs/crate/reqwest/0.10.8/source/src/blocking/wait.rs to share common logic between the async impl and blocking impl?
There was a problem hiding this comment.
I don't understand what the linked code has to do with duplication. I tried to reduce duplication as much as possible by breaking common code into their own functions which are called inside both async_push and push. But this can only go so far, because the underlying reqwest structs use different types.
adamchalmers
force-pushed
the
master
branch
2 times, most recently
from
86a0f40
to
69066dd
Compare
Signed-off-by: Adam Chalmers <[email protected]>
|
Sorry I forgot about this for so many months. I've addressed most of the feedback except the one comment I didn't understand. I'd like to get this merged, because my project is stuck on Prometheus 0.7, which means I'm stuck pulling in an insecure version of Hyper (via reqwest), which means that my work's CI blocks builds. I can always fork this repo until the PR is merged, but I'd rather get this addressed :) |
|
PR is no longer necessary because I realized that Tokio 1.0 doesn't clash with reqwest::blocking! |
|
Actually, the latest reqwest 0.11 and the latest tokio 1 are incompatible again. I think this PR is ultimately the best way forward, that way projects which use rust-prometheus can make their own choice of async or blocking. Apologies for all the back-and-forth on this. |
|
What's the status of this PR? It seems odd, to not provide an async interface when using |
|
Any update on this PR? Would be great to see this functionality land 🙏🏼 |
|
I agree, it would be great for one of the maintainers to review this and tell me what is blocking merge. |
|
Closing as apparently no maintainers want this reviewed. Ping me if you want it reopened. |
I tried to document the problem thoroughly on the issue itself. This is a proposed solution. I tried to share as much code as I could between
push_asyncandpush. Let me know if you'd rather I take a different approach!Fixes: #342