Repository navigation
[Debugger] Gate template dumps on the runtime type - #9541
Draft
dudikeleti wants to merge 6 commits into
Draft
dudikeleti wants to merge 6 commits into
dudikeleti wants to merge 6 commits into
Conversation
…p checks at compile time
… formats customer values
BenchmarksBenchmark execution time: 2026-10-27 20:31:48 Comparing candidate commit 0a8b7fa in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 11 known flaky benchmarks, 115 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9541) and master. ✅ No regressions detected |
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
Log-template dumps (
ProbeExpressionParser.Dump.cs) now use the value's runtime type to decide two things:ToString(),Exception.MessageorException.StackTraceon it;#9483 did this for dictionaries only. This PR covers lists and arrays, exceptions,
ToString()and the null check, and adds runtime-type redaction when the declared type isn't sealed.Reason for change
AddLocalAndArgsusesValue.GetType()). A value reached throughgetmemberor an object field keeps its declared type, which can be an interface or a non-sealed type holding a customer implementation. That implementation's enumerator,Message/StackTraceoverride orToString()override then ran on the application thread while the log message was rendered.object, an interface or a base class was dumped field by field.Expression.Equal, which binds to a customeroperator ==.Implementation details
Wherever the declared type allows, the decision is made when the expression compiles. A sealed or value type is its own runtime type, so it gets no runtime check.
Root values (
DumpExpression) whose declared type isn't sealed. The value is read once, thenIsRedactedRuntimeValue(value, declaredType)(from #9483) runs first and renders{redacted}without an evaluation error. It skips the lookup when the runtime type equals the declared type. After that:Redaction.IsSupportedCollection(value)accepts the runtime type.object, so this is decided at compile time. A class on the allowlist enumerates only whenvalue.GetType() == declaredType. Any other class gets the field dump. Arrays are unchanged.Uri,StackTraceToString()only whenvalue.GetType() == declaredType.DumpException(see below). A declared type outside CoreLib skips the runtime check, because no CoreLib type can derive from it.A runtime type that is rejected gets the field dump, the same output a root local of that type gets.
DumpException:Type, Message, StackTrace. Two CoreLib types are excluded because theirMessageformats values that can be customer objects:ReflectionTypeLoadExceptionreads its loader exceptions'Message, andSwitchExpressionExceptionformats the unmatched value.Type, <message>, where the message isException._messageread through a cachedFieldInfo, so no override runs. When that field is null, justType.Null check:
Expression.ReferenceEqual, or!HasValueforNullable<T>.Fields (
DumpObject):IsTypeSupportIndex) runs only after that. Only allowlisted runtime types are enumerated. [Debugger] DEBUG-5963 Dump dictionaries in log templates with key-based redaction #9483's runtime-type redaction is unchanged.ToString()is called only when the runtime type equals the declared type, or when the declared type is a value type.DumpException.Removed:
IsSafeException. It has no callers left, and its namespace-based rule is what this PR replaces.Per-evaluation cost
GetType()and a reference compare. OneRedaction.IsRedactedTypelookup, only when the runtime type differs from the declared one.Redaction.IsSupportedCollection(value), which is allocation-free.GetType()reference compare, or nothing if the class isn't allowlisted.Uri/StackTraceGetType()reference compare.GetType().Assemblyand a reference compare, plus 2 reference compares against the two excluded CoreLib types.FieldInforead ofException._messageinstead of callingMessage/StackTrace. Only for exceptions outside CoreLib.IsTypeSupportIndex(namespaceStartsWith,GetInterface×2,GetInterfaces().Any(...), which allocates) now runs only for null values and allowlisted runtime collections, not for every field.ToString()GetType()reference compare.IsMicrosoftType). + the assembly compare. Redaction lookup only when the runtime type differs.There are no new
GetInterfaces()calls, LINQ, closures or string building on the evaluation path; theFunclambdas run only at compile time.The redaction lookup is the existing
ConcurrentAdaptiveCache, which takes a lock.dudik/perf/redaction-cache-lock-freemakes a cache hit lock-free and allocation-free: 707 ns / 454 B before, 10.9 ns / 0 B after.Behavior and output changes
List<string>subclass behindIList<string>, aUrisubclass, or a customerDictionary<,>subclass:ReadOnlyCollection<T>,Collection<T>,ObservableCollection<T>, immutable collections, ...) behind a collection-typed root value are field-dumped instead of enumerated. Snapshot capture already treats them this way.IList/IReadOnlyList<>whose runtime type isn't allowlisted show the declared type name instead of being enumerated. If such a runtime type is also configured as redacted, the field shows the declared type name instead of{redacted}; neither shows the content.{redacted}. Before, the redacted type's fields were dumped.{@exception}now rendersSystem.InvalidCastException, <message>, <stack>instead ofSystem.Exception, ...(see theExceptionsnapshot).Type, <message>without a stack trace. That covers customer exceptions and Microsoft ones outside CoreLib such asXmlExceptionandHttpRequestException.Type, <message>. Before, it got a field dump, which is usually empty.ReflectionTypeLoadExceptionandSwitchExpressionExceptionrenderType, <message>.Uri/StackTracerendersnull. Before, it raised an evaluation error.AccessNullableStruct,AccessNullableStructNotNull,Collection,CustomArrayAtIndex,ExceptionandToStringNotSupported. OnlyException's rendered result changes.Test coverage
New tests in
DebuggerExpressionLanguageTests:TemplateDump_GetMemberDumpsFieldsOfCustomerRuntimeType:getmemberon fields declaredList<string>,IList<string>,IReadOnlyList<string>,Dictionary<,>andUri. They hold customer subclasses whose enumerators andToString()throw.TemplateDump_FieldsOfCustomerRuntimeTypeDumpDeclaredTypeName: the same values as fields.TemplateDump_GetMemberDumpsAllowedRuntimeType: aList<string>behindIList<string>, an exactUri, and anXmlExceptionbehind a declaredXmlExceptionare still dumped.TemplateDump_GetMemberCollectionClassOutsideAllowlistDumpsFields:Collection<string>, whose dump is decided at compile time.TemplateDump_GetMemberExceptionFieldandTemplateDump_ExceptionFieldsCallMembersOnlyOnCoreLibRuntimeType, over four exceptions:InvalidOperationExceptionwhoseMessageandStackTracethrow;XmlException, which is outside CoreLib.TemplateDump_CustomerExceptionLocalShowsMessageField.TemplateDump_ReflectionTypeLoadExceptionDoesNotReadLoaderExceptionMessages: as a local, throughgetmemberand as a field.TemplateDump_SwitchExpressionExceptionDoesNotFormatUnmatchedValue, on .NET Core only.TemplateDump_NullCheckDoesNotInvokeEqualityOperator: a root value whose type has a throwingoperator ==.RuntimeTypeRedactionTestsclass:GetMemberNonSealedField_RedactsConfiguredRuntimeType: anobjectfield holding a redacted type, a redactedList<string>behindIList<string>, a redacted exception behindException, and a non-redactedobjectfield.ExceptionField_RedactsConfiguredRuntimeType.Results on this branch:
dotnet test tracer/test/Datadog.Trace.Tests/Datadog.Trace.Tests.csproj -c Release -f net10.0 --filter "FullyQualifiedName~Datadog.Trace.Tests.Debugger": 919 passed, 2 skipped.-f net48: 882 passed, 29 skipped.Other details
dudik/perf/redaction-cache-lock-free, which isn't published yet. It makes the runtime-type redaction lookup cheap. This PR doesn't need it to build, but it should stay a draft until that one merges.ShouldRedactDumpValuetoShouldRedactValue.Messagegetters still format customer values:AggregateException(on .NET Core it appends each inner exception'sMessage) andArgumentOutOfRangeException(it formatsActualValue). They could take the same_messagepath, butAggregateExceptionwould then lose the inner messages it shows today.BlockingCollection<T>is allowlisted, but it enumerates its backingIProducerConsumerCollection<T>, which can be a customer type. This affects snapshot capture too.ConcurrentBag<T>/ConcurrentQueue<T>enumeration takes locks and copies the collection.List<int>,int[]) already fail to compile before this PR.any/all/filter/indexstill gate on the declared type. That is being handled in other PRs.Message/StackTraceon any exception type.Redaction.IsRedactedTypeallocates a delegate per call from a method group. Check whether the cache PR fixes that.