Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove rental crate and implement the required functionality #6625

Closed
wants to merge 4 commits into from

Conversation

mattrutherford
Copy link
Contributor

Closes #6312

Introduces new struct ProxiedSpan that replaces SpanAndGuard that was used in conjunction with rental.

This allows us to remove the now unmaintained rental crate.

@mattrutherford mattrutherford added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 10, 2020
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 10, 2020
@mattrutherford mattrutherford added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jul 10, 2020
@mattrutherford mattrutherford changed the title [WIP] Replace rental crate and implement the required functionality [WIP] Remove rental crate and implement the required functionality Jul 10, 2020
@mattrutherford mattrutherford changed the title [WIP] Remove rental crate and implement the required functionality Remove rental crate and implement the required functionality Jul 13, 2020
@mattrutherford
Copy link
Contributor Author

This new code essentially replicates what the rental crate was doing, just with a strictly limited interface. I think it should soon be possible to replace the need for this altogether, so perhaps it's worth leaving things as is with the rental crate for now. However I don't think there would be any downside to merging this, if deemed desirable.

@mattrutherford mattrutherford added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 14, 2020
@bkchr
Copy link
Member

bkchr commented Aug 5, 2020

@mattrutherford Do we still need this?

@mattrutherford
Copy link
Contributor Author

mattrutherford commented Aug 5, 2020

It's uncertain, but possible that we still need to do this for WASM, because as of right now, experiments to access the spans directly via a new interface breaks the relationship between spans from wasm and corresponding native tracing spans/events. It's possible to avoid this using our custom subscriber if we switch to use 2 Events (enter/exit) instead of a single Span and patch them to our span representation in the subscriber, but that wouldn't preserve the parent_id relationships if we adopt the tracing_subscriber::FmtSubscriber in future.

However maybe OK to close this PR for now seeing as there's no WASM tracing implementation currently using it yet - we can bring these changes into a future PR that introduces it if still needed by then.

@bkchr bkchr closed this Aug 10, 2020
@bkchr bkchr deleted the matt-tracing-proxy branch August 10, 2020 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace rental crate in sp_tracing
2 participants