Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow the agent to accept multiple versions of legacy NR distributed tracing headers #1489

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

nr-ahemsath
Copy link
Member

@nr-ahemsath nr-ahemsath commented Mar 29, 2023

Description

Resolves #863. The agent will now accept incoming legacy distributed tracing headers where the key is any of the following: newrelic, Newrelic, NEWRELIC.

There were some performance concerns associated with doubling the number of potential getter calls when there are no tracing headers present, such as the initial request to a web front-end application if the browser is not instrumented. I did the following test:

Created standard ASP.Net Core web api app (dotnet new webapi), instrumented with agent (either latest release or this branch)
Made requests to http://localhost:5000/WeatherForecast as fast as possible for five minutes and counted the number of requests completed in that time.
Used dotnet-counters to collect other performance data (CPU/memory/GC, etc.)
Results:
Main branch:
1277099 requests completed
PR branch:
1265241 requests completed
No significant difference in CPU, memory, etc.

This PR also removes dead code associated with the obsoleted AcceptDistributedTracePayload API. 🧹

Author Checklist

Reviewer Checklist

  • Perform code review
  • Pull request was adequately tested (new/existing tests, performance tests)
  • Review Changelog

@chynesNR
Copy link
Member

The changes look good, but I'm a little wary of removing API methods without a major rev. It's true they've been deprecated for years, and anyone using them has been getting Warnings, but it's possible someone left them in and has been ignoring them. I'll go with the majority, though, if they think it's low-risk.

@tippmar-nr
Copy link
Member

Regarding removing the deprecated public APIs, I don't have enough history to understand how long they've been "unused." It would be unfortunate to blow a customer out of the water if they happened to still be using the APIs, in spite of all the warnings against it.

But...

It feels like the changes that remove the APIs are actually bigger than (and, unless I'm missing something, unrelated to) the bug that is being resolved with this PR. To that end, I think I'd prefer to split the work out into two separate PRs.

@nr-ahemsath
Copy link
Member Author

The changes look good, but I'm a little wary of removing API methods without a major rev. It's true they've been deprecated for years, and anyone using them has been getting Warnings, but it's possible someone left them in and has been ignoring them. I'll go with the majority, though, if they think it's low-risk.

If you look at

public void AcceptDistributedTracePayload(string payload, int transportType)
you'll see that since 9.0, anybody calling that API from their application just gets a warning and nothing functional happens.

@nr-ahemsath
Copy link
Member Author

Regarding removing the deprecated public APIs, I don't have enough history to understand how long they've been "unused." It would be unfortunate to blow a customer out of the water if they happened to still be using the APIs, in spite of all the warnings against it.

See my reply to @chynesNR's similar question. The warning will remain in place so there should be no functional change to customers still (inadvertently, we'd hope) using the obsoleted API.

@nr-ahemsath nr-ahemsath merged commit 23ee241 into main Mar 31, 2023
@nr-ahemsath nr-ahemsath deleted the bugfix/863-flexible-accept-dt-header-name branch March 31, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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