Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(metrics): update macros after update #2062

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tessus
Copy link
Contributor

@tessus tessus commented May 30, 2024

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@tessus
Copy link
Contributor Author

tessus commented May 30, 2024

based off #2061

@tessus
Copy link
Contributor Author

tessus commented May 30, 2024

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.

@ellie
Copy link
Member

ellie commented May 31, 2024

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.

Cargo.lock Outdated Show resolved Hide resolved
@ellie
Copy link
Member

ellie commented May 31, 2024

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

@tessus
Copy link
Contributor Author

tessus commented May 31, 2024

Ah, I see. I saw that only after I posted my previous reply. I guess this means metrics can only be updated, when the referenced issue has been implemented.

@tessus
Copy link
Contributor Author

tessus commented May 31, 2024

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.

@ellie
Copy link
Member

ellie commented May 31, 2024

Happy to merge this once metrics-rs/metrics#420 or similar are in

@tessus
Copy link
Contributor Author

tessus commented May 31, 2024

Makes perfect sense. see also #2062 (comment)

@LecrisUT
Copy link
Contributor

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

@LecrisUT
Copy link
Contributor

@tessus can you drop the first commit so that this PR is more focused? I've made #2065 instead for that.

@LecrisUT
Copy link
Contributor

LecrisUT commented Jun 1, 2024

@tessus can you drop the first commit so that this PR is more focused? I've made #2065 instead for that.

I'm concerned that the revert commit will do an actual revert when it's being merged. Could you rebase and drop the commit instead?

@tessus
Copy link
Contributor Author

tessus commented Jun 1, 2024

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.

@tessus
Copy link
Contributor Author

tessus commented Jun 5, 2024

The maintainer of metrics hssn't even replied to my PR yet. I guess this might take a while...

@tessus
Copy link
Contributor Author

tessus commented Jun 18, 2024

FYI: my PR has been merged. waiting for a new metrics-exporter-prometheus release...

@tessus tessus force-pushed the fix/metrics branch 2 times, most recently from 044fcd6 to c396f20 Compare June 24, 2024 00:54
@tessus
Copy link
Contributor Author

tessus commented Jun 24, 2024

@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.

@ellie
Copy link
Member

ellie commented Jun 24, 2024

Thanks!

It looks like rustls switched from ring to aws-lc-rs, which is causing the issues

@tessus
Copy link
Contributor Author

tessus commented Jun 24, 2024

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

@LecrisUT
Copy link
Contributor

LecrisUT commented Sep 9, 2024

I just read the comments again. It sounds like this can be fixed by using the ring feature of rustls instead. Wouldn't it work if the dependency is made to be axum-server/tls-rustls-no-provider + rustls/ring?

@tessus
Copy link
Contributor Author

tessus commented Sep 9, 2024

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.

@LecrisUT
Copy link
Contributor

LecrisUT commented Sep 9, 2024

Oh right, didn't see that #2382 basically does that. 🤞

@tessus
Copy link
Contributor Author

tessus commented Sep 9, 2024

@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

@LecrisUT
Copy link
Contributor

LecrisUT commented Sep 9, 2024

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 axum-server needs to be implemented for metrics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants