Add include/exclude settings for Hybrid Agent tracers#1681
Conversation
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1681 +/- ##
==========================================
+ Coverage 81.85% 81.88% +0.03%
==========================================
Files 214 214
Lines 25683 25702 +19
Branches 4076 4075 -1
==========================================
+ Hits 21022 21047 +25
+ Misses 3264 3262 -2
+ Partials 1397 1393 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hmstepanek
left a comment
hmstepanek
left a comment
There was a problem hiding this comment.
A couple minor things and I had a question about the tests-I want to make sure we are validating the include and the exclude functionality.
|
|
||
| def _environ_as_comma_separated_set(name, default=""): | ||
| value = os.environ.get(name, default) | ||
| if value in [None, ""]: |
There was a problem hiding this comment.
I think the if not string inside the parser will catch both of these already so this is redundant.
| if value in [None, ""]: |
| def _environ_as_comma_separated_set(name, default=""): | ||
| value = os.environ.get(name, default) | ||
| if value in [None, ""]: | ||
| return set() |
There was a problem hiding this comment.
| return set() |
| internal included defaults, to determine which tracers | ||
| should be used. | ||
| """ | ||
| if not _settings.opentelemetry.enabled or not _is_installed("opentelemetry-api"): |
There was a problem hiding this comment.
I think this check is already present in the _process_opentelemetry_instrumentation_entry_points that calls this so this one might not be necessary. Also _is_installed may be a bit slow in some cases so it's better to call this as little as possible.
| if not _settings.opentelemetry.enabled or not _is_installed("opentelemetry-api"): |
There was a problem hiding this comment.
Good call--this only gets called from _process_opentelemetry_instrumentation_entry_points so we're good on that front.
| should be used. | ||
| """ | ||
| if not _settings.opentelemetry.enabled or not _is_installed("opentelemetry-api"): | ||
| return |
There was a problem hiding this comment.
| return |
| @@ -22,7 +22,7 @@ | |||
| "debug.log_data_collector_payloads": True, | |||
There was a problem hiding this comment.
We should have some tests that validate the exclude in here too. Unless we do have them and I missed them?
There was a problem hiding this comment.
It's a little hard to tell, but the exclude tests are here: https://github.com/newrelic/newrelic-python-agent/pull/1681/changes#diff-435c3f1f807ec58cefd29e84a328657aa6f0f188b28586eb1e650e4999ba9042R99
Using the tests in the OpenTelemetry frameworks that are also supported in New Relic seemed to be the way to go for testing the include functionality while the exclude tests were done on custom tracers.
3 participants
This PR adds the following settings:
opentelemetry.traces.include(config file) orNEW_RELIC_OPENTELEMETRY_TRACES_INCLUDE(environment variable)opentelemetry.traces.exclude(config file) orNEW_RELIC_OPENTELEMETRY_TRACES_EXCLUDE(environment variable)Because these settings have been added, the default behavior of the agent with conflicting instrumentation has now changed. If New Relic and OpenTelemetry both support a framework, the Hybrid Agent will only use the OpenTelemetry instrumentation library if it is explicitly included in the include list. The only frameworks that are enabled by default when OpenTelemetry is enabled are those that are only supported by OpenTelemetry and not New Relic.