Repository navigation
Change Tag to hold a String instead of Cow<str> - #52
Merged
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
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Oct 26, 2026
# What does this PR do? Adds `Disabled`, `Immediate`, and `FutureExperimentsOnly` evaluation modes using `dd_experimentation_opt_out_at`. SDK wrappers resolve provider configuration; the evaluator reads no process settings. - Immediate mode skips exposure-logging allocations before targeting and randomization. - Future mode retains activation strictly before the cutoff. Equal, later, missing, or null activation excludes the allocation. - Invalid activation returns per-flag `PARSE_ERROR` only for future mode with a valid cutoff, including unreachable allocations. - Cutoff and activation share the documented ASCII timestamp grammar: valid Gregorian fields, lowercase t/z, nanosecond truncation, and normalized UTC years 0000–9999. Whitespace, named zones, compact offsets and leap seconds are rejected; allocation schedules retain their existing parser. - Ordinary allocations and retained exposure eligibility remain intact. Existing C entry points retain their default and boolean-mode behavior. # Motivation Customers need to exclude subjects from experimentation immediately or from experiments activated at or after their opt-out time. # Additional Notes The parser refinement follows the [review feedback](#2698 (comment)) and the scoped D005/D006/D007 contract. The cross-SDK lowercase disagreement remains separately tracked. # How to test the change? Native tests cover all 90 shared policy cases, malformed input, unreachable allocations, valid neighboring flags, timestamp boundaries, inactive exclusion calls, and panic containment. Shared fixtures use the merged #52 commit. | Tested commit | Validation | Result | Evidence | | --- | --- | --- | --- | | `DataDog/libdatadog@ead1d3d` | Packaged C integration: mode/cutoff/error boundaries, allocation metadata, exposure eligibility, and legacy APIs; macOS/arm64 | 252 cases and 504 calls passed; 1 FFE example passed. 9 null/array/object attribute cases cannot be represented by the C API. | [Native receipt](https://github.com/DataDog/ffe-system-test-data/blob/98f1f5a4aab6e92fa0b74787cee94e36cc7c8e80/contracts/experimentation-opt-out/evidence/libdatadog-adoption-20261023/native-receipt.json) | Downstream SDK runtime and telemetry delivery were not tested. [Shared decisions and scoped adoption](https://github.com/DataDog/ffe-system-test-data/blob/98f1f5a4aab6e92fa0b74787cee94e36cc7c8e80/contracts/experimentation-opt-out/libdatadog-adoption.md). Parser follow-up at [`975c8e4fe`](975c8e4): all 50 timestamp cases pass through both production paths across 464 evaluations, including all seven reviewer examples. The new adapter fails against the prior parser. All 110 native tests pass, including the unchanged 90 policy cases and FFI tests; check, formatting, Clippy, docs and shared-model verification pass. The C integration receipt above remains historical evidence for its stated commit, not this follow-up. [Parser validation and publication receipt](https://github.com/DataDog/ffe-system-test-data/blob/64ea4a312f2167c7d48c7df642ae1e5d0a9327de/contracts/experimentation-opt-out/evidence/libdatadog-parser-20261026/publication.json). Co-authored-by: leo.romanovsky <leo.romanovsky@datadoghq.com>
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
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.