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

subscriber: fix span data for new, exit, and close events #1334

Merged
merged 8 commits into from
Apr 7, 2021

Conversation

akinnane
Copy link
Contributor

Motivation

Span on_events don't display correct span data when using JSON formatter and the on_event is triggered from a parent context.
Ref: #1032

[dependencies]
tracing = "0.1.25"
tracing-subscriber = "0.2.17"
use tracing_subscriber::fmt::format::FmtSpan;

fn main() {
    tracing_subscriber::fmt()
        .json()
        .with_current_span(true)
        .with_span_list(true)
        .with_span_events(FmtSpan::FULL)
        .init();

    tracing::info_span!("parent",  foo=42).in_scope(|| {
        tracing::info_span!("child", bar=24).in_scope(|| {
            tracing::info!("logline")
        });
    });
}
{"timestamp":"Mar 29 00:26:03.749","level":"INFO","fields":{"message":"new"},"target":"i1032"}
{"timestamp":"Mar 29 00:26:03.749","level":"INFO","fields":{"message":"enter"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"new"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"enter"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"},{"bar":24,"name":"child"}]}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"logline"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"},{"bar":24,"name":"child"}]}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"exit"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"close","time.busy":"281µs","time.idle":"250µs"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"exit"},"target":"i1032"}
{"timestamp":"Mar 29 00:26:03.750","level":"INFO","fields":{"message":"close","time.busy":"854µs","time.idle":"212µs"},"target":"i1032"}

The enter and logline events correctly show the span subject's metadata. However, the new, exit, and close events show the span's parent span's metadata instead. The final exit and close events show no metadata as there is no grandparent span to retrieve metadata from.

This shows the timeline of events and the span context they display.

<--1--↓--------no-span---------8--9-->
   <--2--3--↓--parent----6--7--↑-->
         <--4--child--5--↑-->

1. new   `parent`
2. enter `parent`
3. new   `child`
4. enter `child`
5. info! `logline`
6. exit  `child`
7. close `child`
8. exit  `parent`
9. close `parent`

This is primarily caused because FomatEvents::format_event uses ctx.ctx.current_span() to determine the context for the current event.

I think the new, close and exit events should show the metadata for the span referenced in the event's message, however, these on_event actions exist outside the context of the referenced span and show the metadata for that context/span instead.

Solution

This change tries to use event.parent() to determine the correct span context for the event, falling back to ctx.ctx.current_span() If there is no event.parent() in the current event. It produces the following output where each JSON line displays the event subject's metadata.

{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"new"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"enter"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"new"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"enter"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"},{"bar":24,"name":"child"}]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"testevent"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"},{"bar":24,"name":"child"}]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"exit"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"close","time.busy":"24.2µs","time.idle":"21.4µs"},"target":"i1032","span":{"bar":24,"name":"child"},"spans":[{"foo":42,"name":"parent"}]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"exit"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[]}
{"timestamp":"Mar 29 00:33:36.552","level":"INFO","fields":{"message":"close","time.busy":"76.4µs","time.idle":"78.1µs"},"target":"i1032","span":{"foo":42,"name":"parent"},"spans":[]}

New, exit and close span events are generated while the current
context is set to either `None` or the parent span of the span the
event relates to. This causes spans data to be absent from the JSON
output in the case of the `None`, or causes the span data to reference
the parent's span data. Changing the way the current span is
determined allows the correct span to be identified for these
events. Trying to access the events `.parent()` allows access of the
correct span for the `on_event` actions, while using `.current_span()`
works for normal events.

Ref: tokio-rs#1032
@akinnane akinnane requested review from davidbarsky, hawkw and a team as code owners March 28, 2021 23:51
akinnane and others added 6 commits March 29, 2021 01:12
Based on feedback by @hawkw, I've improved the test for tokio-rs#1333 to parse
the json output. This is more specifc for the bug and allows easier
testing of the different span `on_events`.

Ref: tokio-rs#1333 (review)
Use the `on_records` test method check all events have the correct
context as described in the PR.
@akinnane
Copy link
Contributor Author

akinnane commented Apr 6, 2021

Based on the test feedback in #1333 and the improved test from #1350 I've updated this PR to use the record_works test strategy.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This looks good to me --- I had a couple of non-blocking style suggestions.

tracing-subscriber/src/fmt/format/json.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/format/json.rs Outdated Show resolved Hide resolved
@hawkw hawkw merged commit a75c7b2 into tokio-rs:master Apr 7, 2021
hawkw added a commit that referenced this pull request Apr 16, 2021
* subscriber: fix span data for new, exit, and close events

New, exit and close span events are generated while the current
context is set to either `None` or the parent span of the span the
event relates to. This causes spans data to be absent from the JSON
output in the case of the `None`, or causes the span data to reference
the parent's span data. Changing the way the current span is
determined allows the correct span to be identified for these
events. Trying to access the events `.parent()` allows access of the
correct span for the `on_event` actions, while using `.current_span()`
works for normal events.

Ref: #1032

* Fix style

* subscriber: improve test for #1333

Based on feedback by @hawkw, I've improved the test for #1333 to parse
the json output. This is more specifc for the bug and allows easier
testing of the different span `on_events`.

Ref: #1333 (review)

* subscriber: improve #1334 tests covering all span states

Use the `on_records` test method check all events have the correct
context as described in the PR.

* Apply suggestions from code review

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw mentioned this pull request Apr 16, 2021
hawkw added a commit that referenced this pull request Apr 17, 2021
* subscriber: fix span data for new, exit, and close events

New, exit and close span events are generated while the current
context is set to either `None` or the parent span of the span the
event relates to. This causes spans data to be absent from the JSON
output in the case of the `None`, or causes the span data to reference
the parent's span data. Changing the way the current span is
determined allows the correct span to be identified for these
events. Trying to access the events `.parent()` allows access of the
correct span for the `on_event` actions, while using `.current_span()`
works for normal events.

Ref: #1032

* Fix style

* subscriber: improve test for #1333

Based on feedback by @hawkw, I've improved the test for #1333 to parse
the json output. This is more specifc for the bug and allows easier
testing of the different span `on_events`.

Ref: #1333 (review)

* subscriber: improve #1334 tests covering all span states

Use the `on_records` test method check all events have the correct
context as described in the PR.

* Apply suggestions from code review

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 30, 2021
# 0.2.18 (April 30, 2021)

### Deprecated

- Deprecated the `CurrentSpan` type, which is inefficient and largely
  superseded by the `registry` API ([#1321])

### Fixed

- **json**: Invalid JSON emitted for events in spans with no fields
  ([#1333])
- **json**: Missing span data for synthesized new span, exit, and close
  events ([#1334])
- **fmt**: Extra space before log lines when timestamps are disabled
  ([#1355])

### Added

- **env-filter**: Support for filters on spans whose names contain any
  characters other than `{` and `]` ([#1368])

Thanks to @Folyd, and new contributors @akinnane and @aym-v for
contributing to  this release!

[#1321]: #1321
[#1333]: #1333
[#1334]: #1334
[#1355]: #1355
[#1368]: #1368
hawkw added a commit that referenced this pull request May 1, 2021
# 0.2.18 (April 30, 2021)

### Deprecated

- Deprecated the `CurrentSpan` type, which is inefficient and largely
  superseded by the `registry` API ([#1321])

### Fixed

- **json**: Invalid JSON emitted for events in spans with no fields
  ([#1333])
- **json**: Missing span data for synthesized new span, exit, and close
  events ([#1334])
- **fmt**: Extra space before log lines when timestamps are disabled
  ([#1355])

### Added

- **env-filter**: Support for filters on spans whose names contain any
  characters other than `{` and `]` ([#1368])

Thanks to @Folyd, and new contributors @akinnane and @aym-v for
contributing to  this release!

[#1321]: #1321
[#1333]: #1333
[#1334]: #1334
[#1355]: #1355
[#1368]: #1368
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.2.18 (April 30, 2021)

### Deprecated

- Deprecated the `CurrentSpan` type, which is inefficient and largely
  superseded by the `registry` API ([tokio-rs#1321])

### Fixed

- **json**: Invalid JSON emitted for events in spans with no fields
  ([tokio-rs#1333])
- **json**: Missing span data for synthesized new span, exit, and close
  events ([tokio-rs#1334])
- **fmt**: Extra space before log lines when timestamps are disabled
  ([tokio-rs#1355])

### Added

- **env-filter**: Support for filters on spans whose names contain any
  characters other than `{` and `]` ([tokio-rs#1368])

Thanks to @Folyd, and new contributors @akinnane and @aym-v for
contributing to  this release!

[tokio-rs#1321]: tokio-rs#1321
[tokio-rs#1333]: tokio-rs#1333
[tokio-rs#1334]: tokio-rs#1334
[tokio-rs#1355]: tokio-rs#1355
[tokio-rs#1368]: tokio-rs#1368
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

2 participants