cleaning pass on HotChocolate instrumentation by vandonr · Pull Request #6580 · DataDog/dd-trace-dotnet

vandonr · 2025-01-21T11:36:12Z

Summary of changes

Follow up on #6248
Some cracks in the nullable analysis were pointed by Andrew, I wanted to fix those, and I also noticed that there were a couple weird things that could be ironed out too.

Reason for change

Implementation details

add some checks on things that can be null
remove weird code around NameType

Test coverage

Other details

vandonr · 2025-01-21T11:38:23Z

-            // Get the string value of the NameString
-            // The NameString value can be either another NameString or a string


the property is defined here: https://github.com/ChilliCream/graphql-platform/blob/c162ca29c23acc69cf81a33155d7384ad4dc9d0f/src/HotChocolate/Core/src/Types/Resolvers/IOperation.cs#L30
It's a struct that looks like a nullable (it has HasValue and Value properties), which is itself a nullable, so there is 2 levels of .Value to get, which explains the initial confusion around this I think.

I'm somewhat intrigued and afraid by this, and I'm wondering if we actually have any tests that hit the behaviour 🤔 my concern is because nullable types are really weird 😅 I suspect it is fine in this case, just would be more comfortable if we new we were covering all these paths:

NameValue is null

NameValue.Value is null

NameValue.Value is not null

In fact, I'm almost certain this isn't covered, because the behaviour has changed significantly here 🤔

Before, we always created an operation, and then updated the details of the current span. Now you don't do that at all if the operation name is null... That seems like a potentially breaking change?

how can the .Value be null when they are actual non-nullable struct ?
This is not the latest code that we are commenting on too, it now looks like operation.Name.HasValue ? operation.Name.Value.Value : null
operation is not null/default because we check HasValue before, so here we check name, and then name.Value, any of those can be null.
In any case, we do update the span even if one of those values is null.

Are we talking about this NameString?

https://github.com/ChilliCream/graphql-platform/blob/12.22.6/src/HotChocolate/Core/src/Abstractions/NameString.cs

If so, this is a custom struct, not a Nullable<T>. And this property is a Nullable<NameString>.

Just trying to understand the issue :)

Yes that's right, it's a nullable of a custom struct, but we need a proxy both for the nullable and the NameString struct

Before, we always created an operation, and then updated the details of the current span. Now you don't do that at all if the operation name is null

Yeah, ignore this, got my operation vs operation name's confused 😅

I also got confused by the fact there's actually three layers of potential null, IOperationContext.Operation could be null (which is the extra guard you added), and then Operation.NameValue could be null, and Operation.NameValue.Value could be null 😅

datadog-ddstaging · 2025-01-21T12:07:04Z

Datadog Report

Branch report: vandonr/nit
Commit report: 8300a60
Test service: dd-trace-dotnet

❌ 192 Failed (0 Known Flaky), 541878 Passed, 3557 Skipped, 29h 47m 13.94s Total Time