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

Look up span for event #1428

Closed
nightkr opened this issue Jun 9, 2021 · 0 comments
Closed

Look up span for event #1428

nightkr opened this issue Jun 9, 2021 · 0 comments
Labels
kind/feature New feature or request

Comments

@nightkr
Copy link
Contributor

nightkr commented Jun 9, 2021

Feature Request

Crates

  • tracing-subscriber

Motivation

Currently you need to handle a lot of cases yourself when writing a Layer that tries to map an Event back to its Span stack. To handle both implicit and explicit span association you more or less need the following logic:

if event.is_root() {
  None
} else if event.is_contextual() {
  ctx.lookup_current()
} else {
  event.parent().and_then(|id| ctx.span(id))
}

Otherwise you might get bugs like the following event being associated with the wrong Span!:

let span_a = info_span!("a");
let span_b = info_span!("b").entered();
info!(parent: span_a, "hey");

Aside from being tedious and encoding a lot of logic about how Events work internally, even seasoned Tracing users get this wrong, both informally and in published libraries (note: this is not a dig against @davidbarsky, just an example of the confusion that this can cause).

Motivated by some discussion on Discord, starting at: https://canary.discord.com/channels/500028886025895936/627649734592561152/852143116244221973

Proposal

Add a Context::event_span method that encodes this logic. Add documentation to Context::current_span and Event::parent referring to it.

Alternatives

Another option would be to have tracing::event!("message") (with no explicit parent) roughly equivalent to tracing::event!(parent: tracing::dispatch::get_current().current_span(), "message"), just like tracing::span! does. That would mean that Event::parent always Does The Right Thing, and no new API is required. @hawkw seemed to be against this path, but to be honest I never quite got what the problem is here.

This was referenced Jun 9, 2021
nightkr added a commit to Appva/tracing that referenced this issue Jun 10, 2021
@hawkw hawkw added the kind/feature New feature or request label Jun 15, 2021
hawkw pushed a commit that referenced this issue Jun 22, 2021
## Motivation

Fixes #1429 

## Solution

Implemented as described in #1429, and migrated all internal uses of the
deprecated methods. All tests passed both before and after the migration
(9ec8130).

- Add a new method `SpanRef::scope(&self)` that returns a leaf-to-root
  `Iterator`, including the specified leaf
- Add a new method `Scope::from_root(self)` (where `Scope` is the type
  returned by `SpanRef::scope` defined earlier) that returns a
  root-to-leaf `Iterator` that ends at the current leaf (in other
  words: it's essentially the same as
  `Scope::collect::<Vec<_>>().into_iter().rev()`)
- Deprecate all existing iterators, since they can be replaced by the
  new unified mechanism:
  - `Span::parents` is equivalent to `Span::scope().skip(1)` (although
    the `skip` is typically a bug)
  - `Span::from_root` is equivalent to `Span::scope().skip(1).from_root()`
    (although the `skip` is typically a bug)
  - `Context::scope` is equivalent to
    `Context::lookup_current().scope().from_root()` (although the
    `lookup_current` is sometimes a bug, see also #1428)
hawkw pushed a commit that referenced this issue Jun 23, 2021
## Motivation

Fixes #1428

## Solution

Adds a new `Context::event_span` method as proposed in the issue.

No existing formatters were changed to use it yet, that seems like a
secondary issue (especially if they already work correctly).
nightkr added a commit to Appva/tracing that referenced this issue Jun 24, 2021
Fixes tokio-rs#1429 (forwardport from v0.1.x branch)

Implemented as described in tokio-rs#1429, and migrated all internal uses of the
deprecated methods. All tests passed both before and after the migration
(9ec8130).

- Add a new method `SpanRef::scope(&self)` that returns a leaf-to-root
  `Iterator`, including the specified leaf
- Add a new method `Scope::from_root(self)` (where `Scope` is the type
  returned by `SpanRef::scope` defined earlier) that returns a
  root-to-leaf `Iterator` that ends at the current leaf (in other
  words: it's essentially the same as
  `Scope::collect::<Vec<_>>().into_iter().rev()`)
- Deprecate all existing iterators, since they can be replaced by the
  new unified mechanism:
  - `Span::parents` is equivalent to `Span::scope().skip(1)` (although
    the `skip` is typically a bug)
  - `Span::from_root` is equivalent to `Span::scope().skip(1).from_root()`
    (although the `skip` is typically a bug)
  - `Context::scope` is equivalent to
    `Context::lookup_current().scope().from_root()` (although the
    `lookup_current` is sometimes a bug, see also tokio-rs#1428)
nightkr added a commit to Appva/tracing that referenced this issue Jun 24, 2021
…o-rs#1434)

Fixes tokio-rs#1428 (forward-port from v0.1.x)

Adds a new `Context::event_span` method as proposed in the issue.

No existing formatters were changed to use it yet, that seems like a
secondary issue (especially if they already work correctly).
@hawkw hawkw closed this as completed in f54136c Jun 24, 2021
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
## Motivation

Fixes tokio-rs#1429 

## Solution

Implemented as described in tokio-rs#1429, and migrated all internal uses of the
deprecated methods. All tests passed both before and after the migration
(9ec8130).

- Add a new method `SpanRef::scope(&self)` that returns a leaf-to-root
  `Iterator`, including the specified leaf
- Add a new method `Scope::from_root(self)` (where `Scope` is the type
  returned by `SpanRef::scope` defined earlier) that returns a
  root-to-leaf `Iterator` that ends at the current leaf (in other
  words: it's essentially the same as
  `Scope::collect::<Vec<_>>().into_iter().rev()`)
- Deprecate all existing iterators, since they can be replaced by the
  new unified mechanism:
  - `Span::parents` is equivalent to `Span::scope().skip(1)` (although
    the `skip` is typically a bug)
  - `Span::from_root` is equivalent to `Span::scope().skip(1).from_root()`
    (although the `skip` is typically a bug)
  - `Context::scope` is equivalent to
    `Context::lookup_current().scope().from_root()` (although the
    `lookup_current` is sometimes a bug, see also tokio-rs#1428)
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
…o-rs#1434)

## Motivation

Fixes tokio-rs#1428

## Solution

Adds a new `Context::event_span` method as proposed in the issue.

No existing formatters were changed to use it yet, that seems like a
secondary issue (especially if they already work correctly).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants