integrations-core
integrations-core copied to clipboard
Default to matching all conntrack metrics
What does this PR do?
Default to matching all conntrack metrics when no whitelist or blacklist is configured.
This PR also updates the documentation comments in the network module's configuration file to indicate that the matcher uses regex matching, which means that supplied whitelist/blacklist configuration might match more than what's expected.
For example: Supplying max
in the whitelist will match tcp_max_retrans
(and this corroborates observed behavior).
Motivation
See Case #: 711411 for full context.
The default behavior feels a bit unintuitive. I would expect that not providing a white/black list would, by default, send all metrics.
Similarly, overriding the default behavior leads to permanent management overhead. Examples:
- If a kernel upgrade exposes additional metrics, these would have to be similarly configured and managed).
- If the whitelist/blacklist behavior changed, we'd have to update configuration
Additional Notes
I don't understand why the default behavior was to limit the metrics sent back, so there could very well be a good reason for it. As mentioned on the Support Ticket I filed, my slight preference is to send all metrics back by default rather than default to a filter being applied due to the ongoing need to manage the configuration overrides. I'm open to a discussion regarding this change in default behavior.
Also note that in my local environment the unit tests failed without any changes. Similarly, the integration tests don't currently test the default behavior (my read is that they do test the filtering logic when a white/blacklist is applied), so I don't believe this needs a test update. Let me know if you don't agree and I can add tests.
Review checklist (to be filled by reviewers)
- [x] Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
- [x] PR title must be written as a CHANGELOG entry (see why)
- [x] Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
- [x] PR must have
changelog/
andintegration/
labels attached
Codecov Report
Merging #11684 (a246443) into master (f92b6f3) will increase coverage by
0.01%
. The diff coverage is100.00%
.
Flag | Coverage Δ | |
---|---|---|
network | 79.27% <100.00%> (+2.07%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Hi @fryz 👋 , this PR looks good and makes sense to me. I am not too familiar with the network check, but when you have it collect all conntrack metrics by default, are there any system.net.conntrack.*
metrics that are not listed in the metadata.csv? These metrics were added a while back so I'm not sure if there are maybe any new ones added that we didn't include originally.
From this PR that added the conntrack metrics, we defaulted to only count
and max
since we believed most customers would only want to see those metrics.
Closing this PR since there hasn't been any activity and there are now branch conflicts.