-
Notifications
You must be signed in to change notification settings - Fork 731
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
opentelemetry: allocate fewer strings for recording events #1917
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
Currently, the OpenTelemetry subscriber calls `level.to_string()` to produce a string that's added as a key-value pair when recording `tracing` events. This means a new `String` will be allocated for _all_ events. This can be avoided by using `Level::as_str` instead, which returns an `&'static str`. Because `opentelemetry`'s `String` values are `Cow`'s, we can use the `&'static str` without allocating a `String`. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
jtescher
approved these changes
Feb 9, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
hawkw
added a commit
that referenced
this pull request
Feb 11, 2022
## 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
## 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
## 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
# 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
added a commit
that referenced
this pull request
Feb 11, 2022
# 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
# 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
pushed a commit
to tokio-rs/tracing-opentelemetry
that referenced
this pull request
Mar 21, 2023
# 0.17.2 (February 21, 2022) This release fixes [an issue][#1944] introduced in v0.17.1 where `tracing-opentelemetry` could not be compiled with `default-features = false`. ### Fixed - Compilation failure with `tracing-log` feature disabled ([#1949]) [#1949]: tokio-rs/tracing#1917 [#1944]: tokio-rs/tracing#1944
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
…1917) ## 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 tokio-rs#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`.
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
# 0.17.1 (February 11, 2022) ### Added - `OpenTelemetryLayer` can now add detailed location information to forwarded events (defaults to on) ([tokio-rs#1911]) - `OpenTelemetryLayer::with_event_location` to control whether source locations are recorded ([tokio-rs#1911]) ### Changed - Avoid unnecessary allocations to improve performance when recording events ([tokio-rs#1917]) Thanks to @djc for contributing to this release! [tokio-rs#1917]: tokio-rs#1917 [tokio-rs#1911]: tokio-rs#1911 Closes tokio-rs#1919
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
# 0.17.2 (February 21, 2022) This release fixes [an issue][tokio-rs#1944] introduced in v0.17.1 where `tracing-opentelemetry` could not be compiled with `default-features = false`. ### Fixed - Compilation failure with `tracing-log` feature disabled ([tokio-rs#1949]) [tokio-rs#1949]: tokio-rs#1917 [tokio-rs#1944]: tokio-rs#1944
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, the
tracing-opentelemetry
subscriber will allocate severalstrings for opentelemetry key-value fields in its
on_event
implementation. Some of these allocations are not necessary, since
opentelemetry::Value::String
can take aCow
, allowing&'static str
sto 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:
on_event
method to subscriber to use&'static str
s forevent targets when possible, similarly to how we did this for source
locations in opentelemetry: forward event metadata #1911. This way, when events were not recorded via the
tracing-log
adapter, we will use the&'static
tracing metadatastring for their targets, rather than allocating a new
String
. NewString
s are only allocated when an event came from thelog
crateand its target is not valid for the
'static
lifetime.Level::as_str
for theLevel
key-value field, instead ofLevel::to_string
.to_string
callsfmt::Display
and returns aString
, whileas_str
returns an&'static str
. This way, levelswill never allocate a
String
.