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

chore: Move FutureExt into context #2776

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

scottgerring
Copy link
Contributor

@scottgerring scottgerring commented Mar 10, 2025

Fixes #2754

Changes

  • Switch opentelemetry::context from context.rs to context/mod.rs module style so that we can add future_ext without jamming everything together into the same file
  • Move FutureExt from opentelemetry::trace::context into opentelemetry::context::future_ext
  • Re-export FutureExt in trace to maintain backwards compatibility

I had to make a couple bits in Context pub(crate) to make this work.

I'm not sure what to call the file containing Context - opentelemetry/src/context/context_store.rs - its contents are re-exported as context::Context as previously available, but double nesting the same module name is a bad practice, and so is putting anything in mod.rs itself. An alternate would be to leave the existing code in src/context.rs, and then use the newer module format to add the new bits within the same module in src/context/future_ext.rs.

Open to suggestions!

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)

@scottgerring
Copy link
Contributor Author

@cijothomas is this what you had in mind?

Copy link

codecov bot commented Mar 10, 2025

Codecov Report

Attention: Patch coverage is 3.84615% with 50 lines in your changes missing coverage. Please review.

Project coverage is 79.7%. Comparing base (9d3a507) to head (b35dec4).

Files with missing lines Patch % Lines
opentelemetry/src/context/future_ext.rs 0.0% 50 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2776   +/-   ##
=====================================
  Coverage   79.7%   79.7%           
=====================================
  Files        123     124    +1     
  Lines      23107   23107           
=====================================
  Hits       18426   18426           
  Misses      4681    4681           

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Future extensions should be in common module, not under trace
1 participant