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.

Sign up for GitHub

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

Jump to bottom

opentelemetry: forward event metadata #1911

Merged
merged 6 commits into from Feb 9, 2022
Merged

opentelemetry: forward event metadata #1911

merged 6 commits into from Feb 9, 2022

Conversation

djc
Copy link
Contributor

@djc djc commented Feb 7, 2022

Fixes #1910.

@djc djc requested a review from jtescher February 7, 2022 15:43
@djc djc requested a review from a team as a code owner February 7, 2022 15:43
@djc
Copy link
Contributor Author

djc commented Feb 7, 2022

The MSRV failure seems unrelated?

@jtescher jtescher added the crate/opentelemetry Related to the `tracing-opentelemetry` crate. label Feb 7, 2022
@hawkw
Copy link
Member

hawkw commented Feb 7, 2022

The MSRV failure seems unrelated?

Yeah, it appears a dependency change broke our MSRV. I'll take care of that.

hawkw
hawkw reviewed Feb 7, 2022
View reviewed changes
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.

overall, this looks good to me; i had some minor suggestions.

tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Feb 7, 2022

@djc FYI, in general, you don't need to force push to your PRs; we merge PRs by squashing anyway, so it's not necessary to squash your work in progress commits. Generally it's a little easier for me to review PRs that don't rewrite their own history and just make work-in-progress changes as separate commits, so don't worry about force pushing.

@djc
Copy link
Contributor Author

djc commented Feb 7, 2022

Ah, sorry, yes.

hawkw
hawkw requested changes Feb 7, 2022
View reviewed changes
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.

i think if we're going to use Cows for non-log events, we should do this even if the tracing-log feature is enabled; a majority of the events are probably still going to be coming from tracing and have 'static metadata, even when the log support is enabled.

tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
@djc
Copy link
Contributor Author

djc commented Feb 9, 2022

(The nightly failures seem unrelated.)

hawkw
hawkw approved these changes Feb 9, 2022
View reviewed changes
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.

okay, this looks good to me --- your approach to handling the tracing-log feature flag is much nicer than my mess of if statements 👍

clippy complains about some redundant closures, so i left suggestions for fixing the lints, but besides that, this looks good to me!

tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
tracing-opentelemetry/src/subscriber.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Feb 9, 2022

Not totally sure what's up with the nightly build failures (that looks wrong to me, possibly a compiler regression of some kind?) but you're right that it's not related to this change.

@hawkw
Copy link
Member

hawkw commented Feb 9, 2022

CI failure looks like a regression on nightly; I reported an upstream issue rust-lang/rust#93821. AFAICT, there's nothing we should do about fixing it, so I'm going to go ahead and merge this PR despite the nightly build failure.

@hawkw hawkw merged commit 2db3750 into master Feb 9, 2022
@hawkw hawkw deleted the otel-event-metadata branch February 9, 2022 18:02
@djc
Copy link
Contributor Author

djc commented Feb 9, 2022

Can I get this into a release? I can backport, bump version and add a change log entry.

@hawkw
Copy link
Member

hawkw commented Feb 9, 2022

@djc normally I take care of all that stuff, but if you'd like to, go ahead! I'll try to get a release out today in any case!

hawkw added a commit that referenced this pull request Feb 9, 2022
@hawkw
opentelemetry: use static strings for event targets
4ac91f6 
This changes the `tracing-opentelemetry` subscriber to use
`&'static str`s for event targets when possible, similarly to how we did
this for source locations in #1911.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw mentioned this pull request Feb 9, 2022
Merged
@hawkw
Copy link
Member

hawkw commented Feb 9, 2022

btw, i noticed some additional allocations we could get rid of: #1917

hawkw added a commit that referenced this pull request Feb 9, 2022
@hawkw
opentelemetry: allocate fewer strings for recording events (#1917)
4e65750 
## Motivation

Currently, the `tracing-opentelemetry` subscriber will allocate several
strings for opentelemetry key-value fields in its `on_event`
implementation. Some of these allocations are not necessary, since
`opentelemetry::Value::String` can take a `Cow`, allowing `&'static str`s
to be used without allocating a new `String`.

Since this happens for _every_ event that's recorded to opentelemetry,
this probably has a meaningful performance impact.

## Solution

This branch makes two primary changes:

+ Change the `on_event` method to subscriber to use `&'static str`s for
  event targets when possible, similarly to how we did this for source
  locations in #1911. This way, when events were not recorded via the
  `tracing-log` adapter, we will use the `&'static` tracing metadata
  string for their targets, rather than allocating a new `String`. New
  `String`s are only allocated when an event came from the `log` crate
  and its target is not valid for the `'static` lifetime.

* Use `Level::as_str` for the `Level` key-value field, instead of
  `Level::to_string`. `to_string` calls `fmt::Display` and returns a
  `String`, while `as_str` returns an `&'static str`. This way, levels
  will never allocate a `String`.
@djc djc mentioned this pull request Feb 10, 2022
Closed
@djc
Copy link
Contributor Author

djc commented Feb 10, 2022 •
edited

@hawkw since I didn't see a release come out, I just submitted #1919 (#1917 looks nice, too!).

hawkw pushed a commit that referenced this pull request Feb 11, 2022
@djc @hawkw
opentelemetry: forward event source locations (#1911)
32a3667 
This branch adds the source code file, module path, and line number to
OpenTelemetry events as the OpenTelemetry `code.filepath`,
`code.namespace`, and `code.lineno` fields, respectively, if they are
set in the `tracing` event's metadata.

Fixes #1910
hawkw added a commit that referenced this pull request Feb 11, 2022
@hawkw
opentelemetry: allocate fewer strings for recording events (#1917)
44c768b 
## Motivation

Currently, the `tracing-opentelemetry` subscriber will allocate several
strings for opentelemetry key-value fields in its `on_event`
implementation. Some of these allocations are not necessary, since
`opentelemetry::Value::String` can take a `Cow`, allowing `&'static str`s
to be used without allocating a new `String`.

Since this happens for _every_ event that's recorded to opentelemetry,
this probably has a meaningful performance impact.

## Solution

This branch makes two primary changes:

+ Change the `on_event` method to subscriber to use `&'static str`s for
  event targets when possible, similarly to how we did this for source
  locations in #1911. This way, when events were not recorded via the
  `tracing-log` adapter, we will use the `&'static` tracing metadata
  string for their targets, rather than allocating a new `String`. New
  `String`s are only allocated when an event came from the `log` crate
  and its target is not valid for the `'static` lifetime.

* Use `Level::as_str` for the `Level` key-value field, instead of
  `Level::to_string`. `to_string` calls `fmt::Display` and returns a
  `String`, while `as_str` returns an `&'static str`. This way, levels
  will never allocate a `String`.
@hawkw hawkw mentioned this pull request Feb 11, 2022
Merged
hawkw pushed a commit that referenced this pull request Feb 11, 2022
@djc @hawkw
opentelemetry: forward event source locations (#1911)
e0c86ee 
This branch adds the source code file, module path, and line number to
OpenTelemetry events as the OpenTelemetry `code.filepath`,
`code.namespace`, and `code.lineno` fields, respectively, if they are
set in the `tracing` event's metadata.

Fixes #1910
hawkw added a commit that referenced this pull request Feb 11, 2022
@hawkw
opentelemetry: allocate fewer strings for recording events (#1917)
ef29d5b 
## Motivation

Currently, the `tracing-opentelemetry` subscriber will allocate several
strings for opentelemetry key-value fields in its `on_event`
implementation. Some of these allocations are not necessary, since
`opentelemetry::Value::String` can take a `Cow`, allowing `&'static str`s
to be used without allocating a new `String`.

Since this happens for _every_ event that's recorded to opentelemetry,
this probably has a meaningful performance impact.

## Solution

This branch makes two primary changes:

+ Change the `on_event` method to subscriber to use `&'static str`s for
  event targets when possible, similarly to how we did this for source
  locations in #1911. This way, when events were not recorded via the
  `tracing-log` adapter, we will use the `&'static` tracing metadata
  string for their targets, rather than allocating a new `String`. New
  `String`s are only allocated when an event came from the `log` crate
  and its target is not valid for the `'static` lifetime.

* Use `Level::as_str` for the `Level` key-value field, instead of
  `Level::to_string`. `to_string` calls `fmt::Display` and returns a
  `String`, while `as_str` returns an `&'static str`. This way, levels
  will never allocate a `String`.
hawkw pushed a commit that referenced this pull request Feb 11, 2022
@djc @hawkw
opentelemetry: forward event source locations (#1911)
f8a93f1 
This branch adds the source code file, module path, and line number to
OpenTelemetry events as the OpenTelemetry `code.filepath`,
`code.namespace`, and `code.lineno` fields, respectively, if they are
set in the `tracing` event's metadata.

Fixes #1910
hawkw added a commit that referenced this pull request Feb 11, 2022
@hawkw
opentelemetry: allocate fewer strings for recording events (#1917)
21eb741 
## Motivation

Currently, the `tracing-opentelemetry` subscriber will allocate several
strings for opentelemetry key-value fields in its `on_event`
implementation. Some of these allocations are not necessary, since
`opentelemetry::Value::String` can take a `Cow`, allowing `&'static str`s
to be used without allocating a new `String`.

Since this happens for _every_ event that's recorded to opentelemetry,
this probably has a meaningful performance impact.

## Solution

This branch makes two primary changes:

+ Change the `on_event` method to subscriber to use `&'static str`s for
  event targets when possible, similarly to how we did this for source
  locations in #1911. This way, when events were not recorded via the
  `tracing-log` adapter, we will use the `&'static` tracing metadata
  string for their targets, rather than allocating a new `String`. New
  `String`s are only allocated when an event came from the `log` crate
  and its target is not valid for the `'static` lifetime.

* Use `Level::as_str` for the `Level` key-value field, instead of
  `Level::to_string`. `to_string` calls `fmt::Display` and returns a
  `String`, while `as_str` returns an `&'static str`. This way, levels
  will never allocate a `String`.
hawkw added a commit that referenced this pull request Feb 11, 2022
@hawkw
opentelemetry: prepare to release v0.17.1
b19aa40 
# 0.17.1 (February 11, 2022)

### Added

- `OpenTelemetryLayer` can now add detailed location information to
  forwarded events (defaults to on) ([#1911])
- `OpenTelemetryLayer::with_event_location` to control whether source
  locations are recorded ([#1911])
### Changed

- Avoid unnecessary allocations to improve performance when recording
  events ([#1917])

Thanks to @djc for contributing to this release!

[#1917]: #1917
[#1911]: #1911

Closes #1919
@hawkw hawkw mentioned this pull request Feb 11, 2022
Merged
hawkw added a commit that referenced this pull request Feb 11, 2022
@hawkw
opentelemetry: prepare to release v0.17.1 (#1922)
f4a44cf 
# 0.17.1 (February 11, 2022)

### Added

- `OpenTelemetryLayer` can now add detailed location information to
  forwarded events (defaults to on) ([#1911])
- `OpenTelemetryLayer::with_event_location` to control whether source
  locations are recorded ([#1911])
### Changed

- Avoid unnecessary allocations to improve performance when recording
  events ([#1917])

Thanks to @djc for contributing to this release!

[#1917]: #1917
[#1911]: #1911

Closes #1919
davidbarsky pushed a commit to tokio-rs/tracing-opentelemetry that referenced this pull request Mar 21, 2023
@hawkw
opentelemetry: prepare to release v0.17.1 (#1922)
2b722bc 
# 0.17.1 (February 11, 2022)

### Added

- `OpenTelemetryLayer` can now add detailed location information to
  forwarded events (defaults to on) ([#1911])
- `OpenTelemetryLayer::with_event_location` to control whether source
  locations are recorded ([#1911])
### Changed

- Avoid unnecessary allocations to improve performance when recording
  events ([#1917])

Thanks to @djc for contributing to this release!

[#1917]: tokio-rs/tracing#1917
[#1911]: tokio-rs/tracing#1911

Closes #1919
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtescher jtescher jtescher left review comments

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
crate/opentelemetry Related to the `tracing-opentelemetry` crate.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Forward line and file information from event metadata into opentelemetry events
3 participants
@djc @hawkw @jtescher