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

Allow using Arc<str> in key-value strings #809

Closed
hawkw opened this issue Jun 7, 2022 · 1 comment · Fixed by #821
Closed

Allow using Arc<str> in key-value strings #809

hawkw opened this issue Jun 7, 2022 · 1 comment · Fixed by #821
Labels
A-common Area:common issues that not related to specific pillar enhancement New feature or request

Comments

@hawkw
Copy link
Contributor

hawkw commented Jun 7, 2022

Currently, opentelemetry represents strings in key-value pairs (i.e. Keys and Value::Strings) using Cow<'static, str>. This has the advantage that when a string constant is used, it isn't necessary to allocate a String on the heap for it. If a string is dynamically constructed at runtime, a String is used instead.

One deficiency of this approach is that oftentimes, a string may be dynamically determined at runtime, but repeated many times in an application's traces. In this case, such a string is represented as a String, and cloning it requires allocating memory and copying all of the string's bytes into that allocation. This has an unfortunate performance cost, especially when the same string is repeated a large number of times. It would be nice if it was also possible to use an Arc<str> to represent strings in key-value pairs. That way, cloning a repeated string to emit it as part of a trace would only require incrementing a reference count, reducing the overhead of using the string (and decreasing overall memory use).

This came up while reviewing a change to the tracing crate's OpenTelemetry integration (tokio-rs/tracing#2135 (comment)).

I think it should be possible to support Arc<str> as a representation for Keys without making a breaking change, since Key is a newtype whose field is private. Although the Key::new constructor takes S: Into<Cow<'static, str>>, we could add a new from_arc (or similar) constructor. Then, the internal representation of Key could be changed to something like this:

pub struct Key(KeyInner);

enum KeyInner {
    Static(&'static str),
    Owned(String),
    RefCounted(Arc<str>),
}

impl Key {
    pub fn new<S: Into<Cow<'static, str>>>(value: S) -> Self {
        Self(match value.into() {
             Cow::Owned(s) => KeyInner::Owned(s),
             Cow::Borrowed(s) => KeyInner::Static(s),
        })
    }

    pub fn from_arc(value: Arc<str>) -> Self {
         Self(KeyInner::RefCounted(value))
    }
    
    // ...
}

Unfortunately, though, a similar change for Value isn't possible without breaking API compatibility, because Value is a pub enum without #[non_exhaustive].

@djc
Copy link
Contributor

djc commented Jun 7, 2022

I think we're still fairly nimble about breaking API, so we can probably go the simple, breaking way.

I definitely agree how Arc makes sense here to make it cheap to reuse dynamic keys.

@TommyCpp TommyCpp added enhancement New feature or request A-common Area:common issues that not related to specific pillar labels Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants