Change Tag to hold a String instead of Cow<str>#52
Merged
Conversation
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.
This allows callers to see if a Tag matches a prefix like:
if tag.as_ref().starts_with("process_id:")
Qard
approved these changes
Sep 14, 2022
pawelchcki
approved these changes
Sep 14, 2022
ivoanjo
pushed a commit
that referenced
this pull request
Jun 18, 2025
* Initial version of CODEOWNERS * update CODEOWNERS with new group names * Update CODEOWNERS
ekump
added a commit
that referenced
this pull request
Mar 26, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What does this PR do?
Change
Tagto hold aStringinstead ofCow<str>.I also implemented
AsRef<str>forTag.Motivation
This originated from a lint on nightly which didn't like
Cow::to_ownedbeing called, and instead callers should use
Cow::cloneorCow::into_owned. However, while inspecting the code with fresh eyes, wesaw that we're always holding a
Cow::Ownedvariant, so just hold theStringinstead.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 leakstechnically but works) than to disable everything on a fork. As part of
supporting fork, I need to fixup the
process_idtag to have the newpid after a fork, and
Tag::as_refwill help with that.Additional Notes
I also noticed that we were fully qualifying two usages of
DateTimeandUtceven though we're publicly using those, so I shortened them.How to test the change?
If your code is calling
Tag::to_ownedthen it will break. It should bereworked into using a
Tag::cloneor call.to_stringon it or similar.