-
Notifications
You must be signed in to change notification settings - Fork 111
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
Exclude system services (self-instrumentation) by default #1536
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1536 +/- ##
==========================================
- Coverage 71.72% 64.35% -7.37%
==========================================
Files 195 195
Lines 19471 19567 +96
==========================================
- Hits 13965 12593 -1372
- Misses 4830 6171 +1341
- Partials 676 803 +127
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We have already a field |
Good point! I will remove the new config property and instead integrate my changes into that existing property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM!! I wonder if there is a way to do this same functionality exclusively in Helm.
docs/sources/configure/options.md
Outdated
|
||
| YAML | Environment variable | Type | Default | | ||
| ------------------------- | ------------------------------- | ------- | ------- | | ||
| `exclude_system_services` | `BEYLA_EXCLUDE_SYSTEM_SERVICES` | boolean | true | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this less dependent to our context, and more expandable, could this be a string
instead of a boolean? Then default it to .*alloy.*|.*otelcol.*|.*beyla.*
, allowing users to unset it (or empty it), or directly changing it to other regexp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM!! I wonder if there is a way to do this same functionality exclusively in Helm.
I've tried exactly that in #1473 but it turned into a mess trying to prevent users from accidentally removing those default excludes if they supplied something else in exclude_services
, so it works in a more robust way here.
To make this less dependent to our context, and more expandable, could this be a
string
instead of a boolean? Then default it to.*alloy.*|.*otelcol.*|.*beyla.*
, allowing users to unset it (or empty it), or directly changing it to other regexp.
That sounds good. Seeing as this is probably a 2.0 feature, I would remove allow_self_instrumentation
and instead just implement exclude_system_services
as a string containing the regex, that should harmonize it a bit.
a5e9ca8
to
3f949a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks again @marevers!
Closes #1454
This replaces the existing attribute
allow_self_instrumentation
with a new attribute calledsystem_exclude_services
. If it is set (it is by default) the exclusion of Beyla, Alloy and OTel Collector executables is automatically added to the ExcludeServices configuration. It must be explicitly set to an empty value (or some other value than the default.*alloy.*|.*otelcol.*|.*beyla.*
) to allow self-instrumentation either through the config file or an environment variable.It also removes the need for the warning in the Helm chart as modifying the exclude_services configuration can no longer lead to users accidentally enabling self-instrumentation when they don't mean to.