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

feat: Refine runtime trait #2641

Merged

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Feb 11, 2025

Contributes to #2643

Changes

This PR cleans-up and simplifies the Runtime trait.

Before:

pub trait Runtime: Clone + Send + Sync + 'static {
    type Interval: Stream + Send;

    type Delay: Future + Send + Unpin;

    fn interval(&self, duration: Duration) -> Self::Interval;

    fn spawn(&self, future: BoxFuture<'static, ()>);

    fn delay(&self, duration: Duration) -> Self::Delay;
}

After:

pub trait Runtime: Clone + Send + Sync + 'static {
    fn spawn<F>(&self, future: F) where F: Future<Output = ()> + Send + 'static;

    fn delay(&self, duration: Duration) -> impl Future<Output = ()> + Send + 'static;
}

The main changes is the drop of the interval, which can be moved to an internal helper method.

Future Improvements:

This trait could even made to be agnostic to single-threaded and multi-threaded runtimes and be simplified to:

pub trait Runtime {
    fn spawn<F>(&self, future: F) where F: Future<Output = ()> + Send + 'static;

    async fn delay(&self, duration: Duration);
}

The trait requirements would have to be specified on the consuming side:

impl<E, RT> PeriodicReaderBuilder<E, RT>
where
    E: PushMetricExporter,
    RT: Runtime + Send + Sync + 'static,
    RT::delay(..): send + 'static
{
   fn new(exporter: E, runtime: RT) -> Self {
      ...
   }
}

However, this requires the return type notation support which is not yet stable.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Sorry, something went wrong.

Copy link

linux-foundation-easycla bot commented Feb 11, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.3%. Comparing base (367e484) to head (2b9931b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...try-sdk/src/trace/sampler/jaeger_remote/sampler.rs 0.0% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2641     +/-   ##
=======================================
- Coverage   79.3%   79.3%   -0.1%     
=======================================
  Files        123     123             
  Lines      22670   22682     +12     
=======================================
+ Hits       17986   17993      +7     
- Misses      4684    4689      +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cijothomas
Copy link
Member

This would be towards # 1 in #2643

@martintmk martintmk force-pushed the u/mtomka/refine-runtime-trait branch from bb37bbe to 56b5903 Compare February 11, 2025 19:52
@martintmk martintmk marked this pull request as ready for review February 12, 2025 08:37
@martintmk martintmk requested a review from a team as a code owner February 12, 2025 08:37
@martintmk martintmk force-pushed the u/mtomka/refine-runtime-trait branch from de0d53a to 9987869 Compare February 17, 2025 09:22
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks!

@cijothomas
Copy link
Member

@martintmk https://github.com/hyperium/hyper/blob/master/src/rt/mod.rs is what Hyper has. Maybe we should also need just that?

@martintmk
Copy link
Contributor Author

@cijothomas:

https://github.com/hyperium/hyper/blob/master/src/rt/mod.rs is what Hyper has. Maybe we should also need just that?

The also have a different trait definition for timers:
https://github.com/hyperium/hyper/blob/master/src/rt/timer.rs

It works for them because the timers are only necessary if one wants to do keep-alive for connections, so they are not bound to their runtime trait definition.

@martintmk martintmk changed the title Refine runtime trait feat: Refine runtime trait Feb 27, 2025
@cijothomas cijothomas merged commit fb74565 into open-telemetry:main Feb 27, 2025
23 of 24 checks passed
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.

None yet

4 participants