fix(metrics): update macros after update #2062
Conversation
|
based off #2061 |
|
The cross compile and nix pipelines failed with openssl, thus it has nothing to do with this change. These 2 pipelines don't seem to be very stable. I've seen them fail on several occasions for other PRs as well. |
They tend to fail for an actual reason related to the code though, both are sensitive to dependency changes. Given the nature of this PR, it is likely related. |
|
There's an open issue here; metrics-rs/metrics#418 We already use rustls, so I'd rather not also require openssl. There's no strong need to update the metrics crate right now |
|
Ah, I see. I saw that only after I posted my previous reply. I guess this means |
|
Anyway, if you want to close this, you should mark or tag it somehow. The changes for the metrics update are valid, so we can come back to it whenever you want to update metrics. |
|
Happy to merge this once metrics-rs/metrics#420 or similar are in |
|
Makes perfect sense. see also #2062 (comment) |
|
First, let me check that these changes pass the tests on Fedora, although I think these are disabled by: %check
# * These tests are skipped because they required a Postgres database to be
# running, which is not possible in the build environment.
%cargo_test -- -- --skip sync --skip change_password --skip multi_user_test --skip registration |
|
Don't worry about it. This will be squashed anyway. Also, even if it weren't the case, a revert would be perfectly fine. Additionally I still have to update the cargo.toml as soon as my PR is merged in the metrics repo and a new release is cut. Then the lock file will also be updated... All good. |
|
The maintainer of metrics hssn't even replied to my PR yet. I guess this might take a while... |
|
FYI: my PR has been merged. waiting for a new metrics-exporter-prometheus release... |
tessus
force-pushed
the
fix/metrics
branch
2 times, most recently
from
044fcd6 to
c396f20
Compare
|
@ellie I created a PR to switch to rustls for the prometheus-exporter and a new release has been cut today. Thus no more openssl. The cross-compile has an issue, but I am not able to fix it. |
|
Thanks! It looks like rustls switched from |
|
Unfortunately I don't think this is something I can fix. I think that there are feature flags in rustls, but unless other crates that use rustls make them available, there is nothing someone who uses such a crate can do. So what is the solution to this? Once more it seems we are in a place called dependency hell. Btw, it would also be interesting to know why bindgen-cli cannot be installed on illumos. I suspect the |
|
I just read the comments again. It sounds like this can be fixed by using the |
|
It might be. I was thinking along the same lines. But I have to wait until the other PR is merged. Then I can try it. |
|
Oh right, didn't see that #2382 basically does that. 🤞 |
|
@LecrisUT nope, doesn't work either. the ring feature would have to be specified in metrics-exporter-prometheus. also unless all other crates that use rustls also use ring instead of the new default, this will never work. P.S.: IMO the illumos image needs an update. But that's out of my purview |
|
From what I've heard this is just how it's supposed to be designed where the library propagate the options until it reaches a leaf. Probably a similar design to |
|
@tessus could you try to rebase the PR and pull in the latest |
|
It seems it needs |
|
Yea, I am done with this. Not sure why I have to take care of illumos when I don't even know why there's a pipeline for it. This is a fool's errant and I don't really care. P.S.: This PR is not the only one that cannot be merged because of this aws-lc issue on illumos. |
|
A few thoughts on how to proceed with this one
Otherwise @tessus - we have an Illumos pipeline because we have a bunch of Illumos users, who contributed the pipeline + several other fixes and features. There's no need to be rude about it. |
|
Fwiw, upstream is now aware of the illumos issues: aws/aws-lc-rs#709, though it seems to me an issue with 👍 for the removal of TLS on the server. But wouldn't there still be a need for a TLS library to act as a client? |
Yep. We won't be able to eliminate it entirely, but we can at the very least reduce how many dependencies on TLS we have. It will make things easier to manage. |
It has new maintainers (same as rustls), so it's still fine for now. Long term, graviola will likely be the way to go (no build script required), but aws-lc works too (with complicated build issues) |
|
An update from |
|
Update from the cross package. They have added a fix for Footnotes |
Checks