Agent should be flexible re: case sensitivity of incoming DT header

[nr-ahemsath]

trafficstars

This issue is a follow-on action from #652.

Per the agent specs (NR internal-only link: https://source.datanerd.us/agents/agent-specs/blob/main/distributed_tracing/New-Relic-Payload.md), agents should accept inbound an inbound distributed tracing header with the key case as any of the following: newrelic, NEWRELIC, Newrelic.

Currently, the full agent's internal method that looks for an inbound NR DT header only looks for newrelic: https://github.com/newrelic/newrelic-dotnet-agent/blob/686a119865cdfb382248828ee41bd7606bacb102/src/Agent/NewRelic/Agent/Core/DistributedTracing/TracingState.cs#L245

We should update this method to be compliant with the agent specs, along with appropriate unit and possibly integration tests.

There is also a constant defined in the API namespace that has the header key listed as Newrelic: https://github.com/newrelic/newrelic-dotnet-agent/blob/2ba99ea91ac20f79af40259e485949c6b3b1aac3/src/Agent/NewRelic.Api.Agent/Constants.cs#L31 This constant appears to be unreferenced anywhere in the agent solution. It might have been put there for users of the old, obsoleted manual DT APIs Insert/AcceptDistributedTracePayload(). The replacement Insert/AcceptDistributedTraceHeaders() APIs do not require this constant. It should be removed for general code cleanliness and also to avoid confusing users.

[workato-integration[bot]]

https://issues.newrelic.com/browse/NEWRELIC-3671

[workato-integration[bot]]

Jira CommentId: 174209 Commented by ahemsath:

This story is a little more complicated than I thought originally.

First, regarding I said over a year ago about removing NewRelic.Api.Agent.Constants.DistributedTracePayloadKey:  It's not referenced in the agent, but it is a public field in a public class in our public agent API and thus we can't remove it without it being a breaking change.  This would be a major release item.  I think we should still do it because it should not be required by 10.0.0+ agent users.  Assuming the other work in this story is completed before a major release, a separate tech debt story will be created.

Second: being able to accept incoming DT payloads with multiple casings of the newrelic header is going to challenging for our agent given how it is architected.  There is no central point in the agent where we compare incoming header values and the header constant.  The method referenced above, TracingState.AcceptDistributedTraceHeaders, is passing "newrelic" as the "key" argument to whatever "getter" function has been plumbed down to it.  The "getter" function is specific to whatever instrumentation wrapper is processing incoming headers, because the wrappers know how to access headers from whatever library they are instrumenting.

Just looking across the codebase, several wrappers are already passing getter functions that look for the key in a case-insensitive way: [https://github.com/newrelic/newrelic-dotnet-agent/blob/a19addd512b834b62e334d81958c15fb00c29af3/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/RabbitMq/HandleBasicDeliverWrapper.cs#L61]

[https://github.com/newrelic/newrelic-dotnet-agent/blob/a19addd512b834b62e334d81958c15fb00c29af3/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/NServiceBus/NServiceBusHelpers.cs#L96]

Others, however, don't: [https://github.com/newrelic/newrelic-dotnet-agent/blob/a19addd512b834b62e334d81958c15fb00c29af3/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNet/Shared/HttpContextActions.cs#L154]

(not an exhaustive list, just examples)

So, the work for this story is to make sure that all getters being passed by wrappers are doing case-insensitive key lookups.  Testing TBD (wrappers are currently not unit tested, and it might be challenging to contrive an integration test scenario where the key for the tracing headers isn't the standard "newrelic").

Performance testing of wrappers that change might be warranted as it's more expensive to iterate over an entire collection and make case-insensitive comparisons rather than a simple dictionary lookup.

 

[workato-integration[bot]]

Jira CommentId: 174293 Commented by ahemsath:

Here's a comprehensive list of wrappers that end up calling TracingState.AcceptDistributedTraceHeaders (via Transaction.AcceptDistributedTraceHeaders -> DistributedTracePayloadHandler.AcceptDistributedTracePayloadHandler):

AspNet/Shared/HttpContextActions - changes are needed to make case-insensitive

AspNetCore/WrapPipelineMiddleware - changes are needed to make case-insensitive

NServiceBus/NServiceBusHelpers - already case-insensitive

Owin/OwinStartupMiddleware - changes are needed

RabbitMq/HandleBasicDeliverWrapper - already case-insensitive

Wcf3/MethodInvokerWrapper - changes are needed

 

[workato-integration[bot]]

Jira CommentId: 176048 Commented by ahemsath:

After consulting with the team we went with the simpler approach of having TracingState.AcceptDistributedTraceHeaders call the getter function passed to it three times, once for each of the three variants of the legacy header key.  Performance testing showed the impact of this to be negligible, even in the case where there are no incoming tracing headers (so the method would get called all three times plus the calls to try to find the W3C headers).