integrations-core icon indicating copy to clipboard operation
integrations-core copied to clipboard

Default to matching all conntrack metrics

Open fryz opened this issue 2 years ago • 2 comments

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/ and integration/ labels attached

fryz avatar Mar 16 '22 17:03 fryz

Codecov Report

Merging #11684 (a246443) into master (f92b6f3) will increase coverage by 0.01%. The diff coverage is 100.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.

codecov[bot] avatar Mar 16 '22 18:03 codecov[bot]

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.

yzhan289 avatar Sep 14 '22 17:09 yzhan289

Closing this PR since there hasn't been any activity and there are now branch conflicts.

yzhan289 avatar Jan 20 '23 14:01 yzhan289