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

tracing: remove Into<Option<Id>> impl for Span #1003

Merged
merged 2 commits into from
Oct 1, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Sep 30, 2020

This doesn't work correctly, since the span is dropped when the function
returns, calling try_close on that span. We could remove the inner
value in this case, but then try_close will never be called on that
ID, leaking the parent. If the span is dropped after the new span is
created, instead, everything will work correctly, because the subscriber
will have already increased its reference count (as it now has a
child). Thus, only &Span is valid here.

Fixes #688

Signed-off-by: Eliza Weisman [email protected]

This doesn't work correctly, since the span is dropped when the function
returns, calling `try_close` on that span. We could remove the inner
value in this case, but then `try_close` will _never_ be called on that
ID, leaking the parent. If the span is dropped *after* the new span is
created, instead, everything will work correctly, because the subscriber
will have already increased its reference count (as it now has a
child). Thus, only `&Span` is valid here.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw added the meta/breaking This is a breaking change, and should wait until the next breaking release. label Sep 30, 2020
@hawkw hawkw added this to the tracing 0.2 milestone Sep 30, 2020
@hawkw hawkw requested a review from a team as a code owner September 30, 2020 17:12
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

LGTM.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw merged commit 95d5511 into master Oct 1, 2020
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 7, 2020
* upstream/master:
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  chore: fix nightly clippy warnings (tokio-rs#991)
  chore: bump all crate versions (tokio-rs#998)
  macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000)
  tracing: prepare to release 0.1.21 (tokio-rs#997)
  core: prepare to release 0.1.17 (tokio-rs#996)
  subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995)
  core, tracing: don't inline dispatcher::get_default (tokio-rs#994)
  core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 16, 2020
…spatch-as-ref-tokio-rs#455

* upstream/master: (28 commits)
  opentelemetry: prepare for 0.8.0 release (tokio-rs#1036)
  docs: add favicon for extra pretty docs (tokio-rs#1033)
  subscriber: fix `reload` ergonomics (tokio-rs#1035)
  chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031)
  opentelemetry: Assign default ids if missing (tokio-rs#1027)
  chore: remove deprecated add-path from CI (tokio-rs#1026)
  attributes:  fix `#[instrument(err)]` in case of early returns (tokio-rs#1006)
  core: remove mandatory liballoc dependency with no-std (tokio-rs#1017)
  chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023)
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Tried to clone Id(1), but no span exists with that ID" when setting parent span
2 participants