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

Skip Async-http instrumentation installation when Traces::Backend::NewRelic is defined #2391

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

tannalynn
Copy link
Contributor

If the traces-backend-newrelic instrumentation is being used, then skip async-http instrumentation installation since this gem provides instrumentation.
https://github.com/newrelic/traces-backend-newrelic

fallwith
fallwith previously approved these changes Jan 10, 2024
kaylareopelle
kaylareopelle previously approved these changes Jan 10, 2024
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I have a few questions, and the answers can all be no 🙂

  • Should we add a changelog entry for this? It might be as close as we get to mentioning the new gem in the main agent's release notes.
  • Would it be helpful to have a comment around line 15 describing where Traces::Backend::NewRelic is defined? I'm worried someone might have trouble finding it since it's not part of the async-http library.
  • Do we want to add a test for this? We don't usually test depends_on blocks, so maybe the answer is no, I'm thinking about accidental regression.

@tannalynn tannalynn dismissed stale reviews from kaylareopelle and fallwith via 3b699da January 11, 2024 17:43
@tannalynn
Copy link
Contributor Author

Nice! I have a few questions, and the answers can all be no 🙂

  • Should we add a changelog entry for this? It might be as close as we get to mentioning the new gem in the main agent's release notes.

I was thinking it would be better to wait until we've released a version of the traces-backend-newrelic before adding something for this to the changelog for whatever the next version of the agent is.

  • Would it be helpful to have a comment around line 15 describing where Traces::Backend::NewRelic is defined? I'm worried someone might have trouble finding it since it's not part of the async-http library.

sure, added one

  • Do we want to add a test for this? We don't usually test depends_on blocks, so maybe the answer is no, I'm thinking about accidental regression.

I was leaning towards not, since we dont normally and it'd add the time of running a whole new multiverse suite. Plus the consequence for it failing is pretty minimal. Since it just won't create the second spans inside of the traces-backend-newrelic gem, but the ones the agent creates would still be present, there isn't a concern about double reporting the external requests.

Copy link
Contributor

SimpleCov Report

Coverage Threshold
Line 93.7% 93%
Branch 71.79% 71%

@tannalynn tannalynn merged commit fd98f88 into dev Jan 11, 2024
28 checks passed
@tannalynn tannalynn deleted the dont_install_async_instrumentation_if_newrelic_traces branch January 11, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants