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

Change Tag to hold a String instead of Cow<str> #52

Merged
merged 2 commits into from
Sep 14, 2022
Merged

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented Sep 13, 2022

What does this PR do?

Change Tag to hold a String instead of Cow<str>.

I also implemented AsRef<str> for Tag.

Motivation

This originated from a lint on nightly which didn't like Cow::to_owned
being called, and instead callers should use Cow::clone or
Cow::into_owned. However, while inspecting the code with fresh eyes, we
saw that we're always holding a Cow::Owned variant, so just hold the
String instead.

Simultaneously, I got a crash report in Rust for the PHP profiler. I
don't support forking, and they forked. Anyway, for this particular
crash it's not much harder to support fork (well, it leaks
technically but works) than to disable everything on a fork. As part of
supporting fork, I need to fixup the process_id tag to have the new
pid after a fork, and Tag::as_ref will help with that.

Additional Notes

I also noticed that we were fully qualifying two usages of DateTime and
Utc even though we're publicly using those, so I shortened them.

How to test the change?

If your code is calling Tag::to_owned then it will break. It should be
reworked into using a Tag::clone or call .to_string on it or similar.

This originated from a lint on nightly which didn't like Cow::to_owned
being called, and instead callers should use Cow::clone or
Cow::into_owned. However, while inspecting the code with fresh eyes, we
saw that we're always holding a Cow::Owned variant, so just hold the
String instead.
@morrisonlevi morrisonlevi requested a review from a team as a code owner September 13, 2022 16:24
This allows callers to see if a Tag matches a prefix like:

    if tag.as_ref().starts_with("process_id:")
Copy link
Contributor

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants