-
Notifications
You must be signed in to change notification settings - Fork 33
Partially stablize runtime metrics #87
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
Conversation
|
Wow, that was quick! Will review this today, thanks! And appreciate the heads up re: the incorrect labeling on rustdocs. I can probably knock out a fix for that, unless you were interested in taking it? |
I could probably take a look at that later this week or next. :) |
|
Cool, let's see who gets to it first. I'll probably do it in one sitting if
I start so low risk of duplication of effort.
…On Tue, Jun 24, 2025, 7:56 AM Ross Sullivan ***@***.***> wrote:
*ranger-ross* left a comment (tokio-rs/tokio-metrics#87)
<#87 (comment)>
I can probably knock out a fix for that, unless you were interested in
taking it?
I could probably take a look at that later this week or next. :)
—
Reply to this email directly, view it on GitHub
<#87 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKWOC6RCBD6KLVTGWITN5ZT3FFRLBAVCNFSM6AAAAACAAMXWGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMBQHA2DENRQGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
jlizen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently our github CI is only building with the tokio_unstable RUSTFLAG set.
Since we now have a new surface area of accidentally removing unstable flag from the wrong metrics, can we add a workflow that runs tests with runtime feature enabled, but the tokio_unstable flag not?
Probably this step is sufficient.
c5c38f6 to
0977b25
Compare
|
I made some fixes from your feedback but I ran out of time to address everything today. So I pushed what I have thus far. I reworked one of the macros in the metrics-rs integration module. I'll address the rest of your feedback later this week or next (thanks for the quick review btw 😄) |
I like it! I'm happy with this approach. If we can apply a similar approach elsewhere (along with tacking on the |
0977b25 to
ba7d086
Compare
|
I pushed some changes to address your feedback. lmk what you think. Also side note: When I was testing my CI changes on my fork i noticed that the |
jlizen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress on this, thanks! Looks like there was a bit more 'untangling the impl blocks' you hadn't gotten to yet?
Also had some feedback on:
- double checking that the docsrs config flag inheritance works as expected
- consolidating macros (if you want, or at least adding new macro for the
RuntimeIntervals) - different approach to simplifying struct instantiation (declare the stable things directly with no cfg flag and
..Default::default(), and then mutate in the unstable fields behind a cfg flag
ba7d086 to
73cf76e
Compare
|
I pushed some fixes to address your comments. Hopefully I addressed all of your feedback (outside of this comment) Let me know if I missed any or you'd like to see other changes |
jlizen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes to the macros and code are great! Everything looks good to me there.
The only remaining item is, we do need to update the rt macros to also include the feature = "rt" gate, since in my testing, it didn't automatically inherit the module-level gate if you specify the unstable gate directly on a particular field/struct/method.
With that in place, we should be all set to merge!
|
Hmm, actually another concern is that I'm not super familiar with github actions, the change you made to the workflow seems fine to me on first read... Let me know if you want a pair of eyes debugging. |
73cf76e to
94263a6
Compare
94263a6 to
f1d3e99
Compare
|
Regarding the hanging CI, if I recall correctly its because Since I split it into 2 job I tested in this on my fork and was able to reproduce by adding a branch protection rule. I think you can update the repo settings before merging to add the new CI jobs as required. |
jlizen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the hanging CI, if I recall correctly its because test-versions is a "required check" in this repository's settings.
Makes sense, thanks for the pointer!
Re: the change to add the rt docsrs flag - thanks for that. It looks like I misinterpreted the previous state though - I see the fields expanding to show the proper docsrs config, but the generated docs still only show tokio-unstable on the various fields. It looks like the docs.rs display itself implicitly scopes the broadest gates to the struct indicator, and then only supplements on fields. You can see the same behavior on the tokio docs.
Anyway we can leave the macro as-is, it's redundant but harmless. Sorry for the confusion.
context: tokio-rs#87 It seems to have been stable at the time of tokio-rs#87, tokio=1.45.1, so I'm not sure why it was marked unstable. (It doesn't appear to be documented wrong either; tokio-rs/tokio#6745 doesn't seem to apply)

This PR partially stabilizes the runtime metrics feature.
As discussed in #86 , the approach is to stabilize
tokio_metrics::RuntimeMonitorand the metrics that are considered stable in tokio while keeping unstable metrics opt-in behind thetokio_unstableconfig.Also while reviewing, you might want to be aware that the tokio docs on docs.rs are incorrectly showing some metrics as stable when they are not. (See tokio-rs/tokio#6745)
closes #86