Repository navigation
[Debugger] Redact values of redacted types in expression member and index access - #9539
Draft
dudikeleti wants to merge 3 commits into
Draft
dudikeleti wants to merge 3 commits into
dudikeleti wants to merge 3 commits into
Conversation
BenchmarksBenchmark execution time: 2026-10-29 15:33:20 Comparing candidate commit 642af28 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 7 known flaky benchmarks, 119 flaky benchmarks without significant changes.
|
dudikeleti
force-pushed
the
dudik/fix/debugger-redacted-metric-no-throw
branch
from
October 28, 2026 10:18
17767da to
9db5c69
Compare
dudikeleti
force-pushed
the
dudik/fix/debugger-redacted-type-member-access
branch
from
October 28, 2026 10:18
aae68b5 to
809ede3
Compare
…ndex access Check the static type of ref, getmember, index and the this, @return, @exception, @it, @key and @value identifiers at compile time, and redact both the source and the resolved member or item when the type is configured as redacted. Match auto-property backing-field names against redaction keywords using the helper the dump path already uses.
Collection, string, comparison and logical operations whose operand is the redacted placeholder now return the placeholder or false at compile time instead of adding a non-redaction error, which would replace the rendered template as the snapshot message. Predicates over a redacted source are parsed only to keep the reader aligned, and their errors are dropped.
…ames Match the source name of any compiler-generated field, such as primary constructor captures, anonymous type fields and hoisted locals, against redaction keywords. Short-circuit len and the index, key, substring bound and string argument operands on the redacted placeholder, and compile a condition that uses a redacted value to false. Share the redaction and filter-state helpers.
dudikeleti
force-pushed
the
dudik/fix/debugger-redacted-metric-no-throw
branch
from
October 29, 2026 14:19
9db5c69 to
93a16cf
Compare
dudikeleti
force-pushed
the
dudik/fix/debugger-redacted-type-member-access
branch
from
October 29, 2026 14:19
809ede3 to
642af28
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9539) and master. ✅ No regressions detected |
dudikeleti
added a commit
that referenced
this pull request
Oct 30, 2026
## Summary of changes Adds Dynamic Instrumentation rules to the `/sdk-review` lens files, so the guardrails in `DebuggerSafetyBoundaries.md` are checked before every push: - `Security-And-Safety.md`: section 3 is now "Dynamic Instrumentation". SEC-3.1 also covers expression-tree property access and static property getters. New rules SEC-3.2 to SEC-3.6 cover runtime-type checks, dictionary keys and compiler-generated names, values computed from redacted values, customer data in evaluation errors and logs, and `Expression.Equal` null checks. - `Performance.md`: new section 5 with PERF-5.1 and PERF-5.2 for per-hit locks and cache hits, and failure output on every hit. - `Correctness.md`: COR-6.4 now says which time bounds are acceptable in tests, flags raising a timeout to fix a flaky test, and limits queued markers to single-consumer queues. This replaces #9414. ## Reason for change The guardrails added to `DebuggerSafetyBoundaries.md` in #9562 are rules we want enforced, and `/sdk-review` is what runs before every push. Keeping them in the lens files instead of `AGENTS.md` keeps `AGENTS.md` small. Each new Dynamic Instrumentation rule comes from a bug fixed in a recent PR, and the COR-6.4 change comes from #9414: | Rule | Fixed in | | --- | --- | | SEC-3.1 (update) | #8878 | | SEC-3.2 runtime type | #9483, #9541, #9547 | | SEC-3.3 dictionary keys and source names | #9536, #9539, #9548 | | SEC-3.4 computed from redacted values | #9485, #9537, #9547 | | SEC-3.5 errors and logs | #9559 | | SEC-3.6 `Expression.ReferenceEqual` | #9541 | | PERF-5.1 per-hit locks, cache hits | #9538 | | PERF-5.2 failure output on every hit | #9536, #9537, #9559 | | COR-6.4 (update) | from #9414, not a bug fix | ## Implementation details SEC-3.1 keeps its ID; the section heading changes from "Customer-process safety" to "Dynamic Instrumentation". The Performance "Don't flag" entry about uncontended per-request locks now points to PERF-5.1, so the two don't conflict for locks shared by every thread that hits a probe. SEC-3.1 still links to `DebuggerSafetyBoundaries.md` for the allowed execution policy. None of these rules depend on #9562, but #9562 points the guide's review checklist to them, so merge this PR first. ## Test coverage Docs only. As in #9472, I ran the rules against commits that introduced the issues. Each run was one independent lens reviewer reading the frozen commit and only its lens file. No reviewer was told which rule was being tested. | Rule | Commit reviewed | Result | | --- | --- | --- | | SEC-3.2 | #8879 | Caught: `ShouldRedactDumpValue(null, expression.Type)` in `ProbeExpressionParser.Dump.cs` decides redaction from the declared type | | SEC-3.4 | #8621 (product code) | Caught: `len(ref password)` and `eq(ref password, "x")` capture values computed from the redaction placeholder | | SEC-3.5 | #8621 (product code) | Caught: capture expression errors send `e.Message` in `evaluationErrors` | | SEC-3.6 | #5543 (product code) | Caught: the `Expression.Equal(expression, Expression.Constant(null))` null check | | PERF-5.1 | #6292 | Caught: every redaction cache hit takes a `ReaderWriterLockSlim` read lock and the LFU policy's write lock, and allocates | | PERF-5.1 | #9538 | Correctly not flagged (the lock-free fix) | | PERF-5.2 | #8725 | Missed: the uncapped metric probe path isn't in that diff | | PERF-5.2 | #8621 (product code) | Flagged a different per-hit failure log in the same block (`Evaluation result should not be null`), not the `Log.Error` in the catch | | COR-6.4 | #9396 | Caught: the flush timeout raised from 5s to 1 minute to fix a flaky test | | COR-6.4 | #9074 | Correctly not flagged (`TaskCompletionSource` with a 30s guard) | Not tested: SEC-3.1's new expression-tree clause and SEC-3.3. Reviewers did point out SEC-3.3's literal-only dictionary key check in code that predates the commits under review. ## Other details I kept only rules a reviewer can match in a diff and that a PR shows were needed. An earlier version had a PERF-5.3 rule for `EvaluationBudget` checks. I dropped it because no PR shows a missed check, and it mostly explained how the budget works; that guidance stays in `DebuggerSafetyBoundaries.md` (#9562). SEC-3.3 was also narrowed to the two shapes a diff shows: dictionary keys checked only when literal, and keywords matched against compiler-generated names.
This branch has not been deployed
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.
Summary of changes
DD_DYNAMIC_INSTRUMENTATION_REDACTED_TYPES) are now redacted wherever an expression uses them, not only when dumped whole. That coversref,getmember,index,this,@return,@it,@keyand@value, and the declared type of the member or item being read.<password>k__BackingField, primary-constructor captures, anonymous-type fields and hoisted locals, are matched against redaction keywords by their source name.Reason for change
{"getmember":[{"ref":"customPii"},"TestValue"]}returned the field of a redacted type.MemberPathExpressionchecked only the member name; the type it passed was the type of the name string. Conditions such aseq(getmember(customPii,"key"),"guess")worked as an oracle.getmember(pii, "<password>k__BackingField")bypassed keyword redaction.Implementation details
this, the return value and every argument and local, so for those roots the static type is the exact runtime type.any,all,filter,isEmpty,len,index, string operations, comparisons, logical operators) return the placeholder orfalseinstead of adding a non-redaction error, which would replace the rendered template as the snapshot message.false; its redaction error setsHasConditionError, as before.ShouldRedactValue(moved from the dump partial to the general one) is shared by dumps and member access.Behavior changes:
{"ref":"x"}for a redacted-type local still renders{redacted}and now also reports a redaction error.thisis a redacted type, its fields,{"ref":"this"}and conditions on it are redacted.@it, renders{redacted}.getmemberno longer redacts because the type of the member-name string is configured as redacted.Test coverage
RedactedTypeTestsclass (in the non-parallelRedactionTestscollection) coversref,getmember,index,@it,@key,@value,this, conditions, collection and string operations over a redacted value, and non-redacted types.dotnet test tracer/test/Datadog.Trace.Tests/Datadog.Trace.Tests.csproj -c Release -f net10.0 --filter "FullyQualifiedName~Datadog.Trace.Tests.Debugger": 962 passed, 2 skipped.-f net48: 926 passed, 29 skipped.Other details
object, interface or base type are checked by their declared type. A redacted runtime type behind such a declaration, including a redactedExceptionsubtype through@exception, is still visible. A runtime check, aGetType()plus a pointer compare, follows in a separate PR stacked on this one; it stays a draft until the lock-free redaction cache PR ([Debugger] Make redaction cache hits lock-free and allocation-free #9538) merges.