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

feat: allow creating SpanTrace from Span directly #1492

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Aug 5, 2021

Motivation

I'm writing a tracing subscriber layer that captures tracing events to display in an in-program UI in a somewhat structured manner; as such I don't want to just use the fmt layer's format-to-a-single-string approach, and instead capture (currently) the event metadata, fields, timestamp, thread, and spantrace, to be displayed in a filterable/searchable/interactable manner later.

Currently, the only way to capture a SpanTrace is via SpanTrace::capture. This (rightfully) captures the SpanTrace of the current execution. However, I'm currently inside Subscribe::on_event; I have the span right here! Even if Span::current happens to be the same during on_event as when the event was fired, it seems more "proper" to create a SpanTrace from the span passed in to me, rather than looking up Span::current.

Solution

The actual code change is almost trivial: all that needs to happen is to expose a SpanTrace::new that takes a Span in addition to the existing SpanTrace::current.

@CAD97 CAD97 requested review from yaahc and a team as code owners August 5, 2021 03:04
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 5, 2021

https://deploy-preview-1492--tracing-rs.netlify.app/tracing_error/struct.spantrace#method.new

(Yes I'm aware that master has introduced breaking changes and I'll either need to backport this (almost trivially) or wait for the ecosystem to update. But I'd also like upstream conformation if this is a reasonable addition (or if I should just use Span::current) before pulling in a patch.)

Copy link
Collaborator

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

looks great to me, but I'll leave the final decision to @hawkw.

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.

looks good to me, thanks!

@@ -68,6 +68,11 @@ pub struct SpanTrace {
// === impl SpanTrace ===

impl SpanTrace {
/// Create a new span trace with the given span as the innermost span.
pub fn new(span: Span) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

we may also want to add a From<Span> for SpanTrace conversion, but we can do that separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually have a reason for not doing so here, though I'm not super certain in the justification:

A SpanTrace is suitably different from Span that I'm not sure that From is clear on the semantics of creating the span trace. While SpanTrace physically only holds onto the single Span, logically the SpanTrace is the whole list of spans, from the innermost span that it holds, to the outermost span. As such, .into() feels a bit wrong.

I think it's fine either way, but that's my reasoning for just providing ::new here.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i was on the fence about it — we can always do that separately, if we decide it's worth having? this seems good to me as is.

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 5, 2021

Fixed the trailing whitespace fmt error (whoops, blame the GH code editor).

@hawkw hawkw merged commit aea1a80 into tokio-rs:master Aug 5, 2021
@CAD97 CAD97 deleted the patch-1 branch August 5, 2021 23:49
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 6, 2021

One small flaw with my plan... subscribe::Context gives a SpanRef, and there doesn't seem to be any consumer-facing way to go from SpanRef to Span.

More specifically, ctx allows getting at the (Id, &'static Metadata<'static>) event's|current span, but as it just holds &'_ impl Collect, there seems to be no way of acquiring the Dispatch required to acquire the full Span.

My problem is ultimately that Subscribe layers are given borrowed access to the collector (via Context), but I want an owning clone to create the owned SpanTrace for the current Event. I would guess that the place to expose that functionality would be SpanRef::to_owned(&self) -> Span or similar, but I'm unsure if that's an API addition that tracing would want to (or even could) do.

(In the meantime I'm just biting the bullet and effectively "pre-rendering" the span trace instead.)

hawkw pushed a commit that referenced this pull request Aug 17, 2021

## Motivation

I'm writing a tracing subscriber layer that captures tracing events to
display in an in-program UI in a somewhat structured manner; as such I
don't want to just use the fmt layer's format-to-a-single-string
approach, and instead capture (currently) the event metadata, fields,
timestamp, thread, and spantrace, to be displayed in a
filterable/searchable/interactable manner later.

Currently, the only way to capture a `SpanTrace` is via
`SpanTrace::capture`. This (rightfully) captures the `SpanTrace` of the
current execution. However, I'm currently inside `Subscribe::on_event`;
I have the span right here! Even if `Span::current` happens to be the
same during `on_event` as when the event was fired, it seems more
"proper" to create a `SpanTrace` from the span passed in to me, rather
than looking up `Span::current`.

## Solution

The actual code change is almost trivial: all that needs to happen is to
expose a `SpanTrace::new` that takes a `Span` in addition to the
existing `SpanTrace::current`.
hawkw pushed a commit that referenced this pull request Aug 17, 2021

## Motivation

I'm writing a tracing subscriber layer that captures tracing events to
display in an in-program UI in a somewhat structured manner; as such I
don't want to just use the fmt layer's format-to-a-single-string
approach, and instead capture (currently) the event metadata, fields,
timestamp, thread, and spantrace, to be displayed in a
filterable/searchable/interactable manner later.

Currently, the only way to capture a `SpanTrace` is via
`SpanTrace::capture`. This (rightfully) captures the `SpanTrace` of the
current execution. However, I'm currently inside `Subscribe::on_event`;
I have the span right here! Even if `Span::current` happens to be the
same during `on_event` as when the event was fired, it seems more
"proper" to create a `SpanTrace` from the span passed in to me, rather
than looking up `Span::current`.

## Solution

The actual code change is almost trivial: all that needs to happen is to
expose a `SpanTrace::new` that takes a `Span` in addition to the
existing `SpanTrace::current`.
hawkw added a commit that referenced this pull request Oct 23, 2021
# 0.2.0 (October 23, 2021)

This is a breaking change release in order to update the
`tracing-subscriber` dependency version to [the v0.3.x release
series][v03].

### Changed

- Updated `tracing-subscriber` dependency to [v0.3.0][v03] ([#1677])

### Fixed

- Disabled default features of the `tracing` dependency so that
  proc-macro dependencies are not enabled ([#1144])
- Documentation fixes and improvements ([#635], [#695])

### Added

- **SpanTrace**: Added `SpanTrace::new` constructor for constructing a
  `SpanTrace` from a `Span` passed as an argument (rather than capturing
  the current span) ([#1492])

Thanks to @CAD97 for contributing to this release!

[v03]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.0
[#635]: #635
[#695]: #695
[#1144]: #1144
[#1492]: #1492
[#1677]: #1677
hawkw added a commit that referenced this pull request Oct 23, 2021
# 0.2.0 (October 23, 2021)

This is a breaking change release in order to update the
`tracing-subscriber` dependency version to [the v0.3.x release
series][v03].

### Changed

- Updated `tracing-subscriber` dependency to [v0.3.0][v03] ([#1677])

### Fixed

- Disabled default features of the `tracing` dependency so that
  proc-macro dependencies are not enabled ([#1144])
- Documentation fixes and improvements ([#635], [#695])

### Added

- **SpanTrace**: Added `SpanTrace::new` constructor for constructing a
  `SpanTrace` from a `Span` passed as an argument (rather than capturing
  the current span) ([#1492])

Thanks to @CAD97 for contributing to this release!

[v03]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.0
[#635]: #635
[#695]: #695
[#1144]: #1144
[#1492]: #1492
[#1677]: #1677
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.2.0 (October 23, 2021)

This is a breaking change release in order to update the
`tracing-subscriber` dependency version to [the v0.3.x release
series][v03].

### Changed

- Updated `tracing-subscriber` dependency to [v0.3.0][v03] ([tokio-rs#1677])

### Fixed

- Disabled default features of the `tracing` dependency so that
  proc-macro dependencies are not enabled ([tokio-rs#1144])
- Documentation fixes and improvements ([tokio-rs#635], [tokio-rs#695])

### Added

- **SpanTrace**: Added `SpanTrace::new` constructor for constructing a
  `SpanTrace` from a `Span` passed as an argument (rather than capturing
  the current span) ([tokio-rs#1492])

Thanks to @CAD97 for contributing to this release!

[v03]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.0
[tokio-rs#635]: tokio-rs#635
[tokio-rs#695]: tokio-rs#695
[tokio-rs#1144]: tokio-rs#1144
[tokio-rs#1492]: tokio-rs#1492
[tokio-rs#1677]: tokio-rs#1677
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

3 participants