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

axum 0.8 #69

Merged
merged 4 commits into from
Oct 14, 2024
Merged

axum 0.8 #69

merged 4 commits into from
Oct 14, 2024

Conversation

yanns
Copy link
Contributor

@yanns yanns commented Oct 5, 2024

@Ptrskay3
Copy link
Owner

Ptrskay3 commented Oct 6, 2024

Thanks!

I think because of the matchit update, we'll need to update that too, and the examples need to be migrated as well.

@yanns
Copy link
Contributor Author

yanns commented Oct 7, 2024

@yanns
Copy link
Contributor Author

yanns commented Oct 9, 2024

I've tested this change on an internal application, and the metrics are working well.

Some metrics values have changed to use the {...} convention instead of :.... so that some dashboards must have been adapted. Something worth mentioning in release notes.

@Ptrskay3
Copy link
Owner

Ptrskay3 commented Oct 9, 2024

I've tested this change on an internal application, and the metrics are working well.

Some metrics values have changed to use the {...} convention instead of :.... so that some dashboards must have been adapted. Something worth mentioning in release notes.

Cool, really appreciate the effort. What about your previous comment? Is it still behaving differently?

@yanns
Copy link
Contributor Author

yanns commented Oct 9, 2024

I've tested this change on an internal application, and the metrics are working well.
Some metrics values have changed to use the {...} convention instead of :.... so that some dashboards must have been adapted. Something worth mentioning in release notes.

Cool, really appreciate the effort. What about your previous comment? Is it still behaving differently?

Yes, no change here.
But maybe it's also broken on main. Something I have to check.

@yanns
Copy link
Contributor Author

yanns commented Oct 9, 2024

I confirm that the application does not behave like it's described in the comment on the main branch.

Calling http://localhost:3000/baz/qux/2
creates the following entry:

axum_http_requests_total{method="GET",status="200",endpoint="/baz/qux/:a"} 1

and not

axum_http_requests_total{method="GET",status="200",endpoint="/baz/qux/2_changed"} 1

as described in the comment.

So the migration to axum 0.8 does not change this behavior.

@Ptrskay3
Copy link
Owner

Well, I created #72 for that bug.

Thanks for the PR. Probably I'll have time to test the changes this weekend, but looks good.

Do you want to write the changelog as well, or should I?

@yanns
Copy link
Contributor Author

yanns commented Oct 10, 2024

Do you want to write the changelog as well, or should I?

I added it: 9de885d

Copy link
Owner

@Ptrskay3 Ptrskay3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@Ptrskay3 Ptrskay3 merged commit ff2dc00 into Ptrskay3:master Oct 14, 2024
5 checks passed
@yanns yanns deleted the axum-0.8 branch October 14, 2024 18:02
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.

2 participants