Refactor snmp walk subcommand

[dplepage-dd]

What does this PR do?

This PR refactors the agent snmp walk subcommand to fix some issues and make parts of it more reusable.

Motivation

I am in the process of making another snmp subcommand, and want to reuse most of the arguments and processing used in snmp walk; this change will make that much easier. I also found some issues with the snmp walk command, which I have fixed.

Changes

Loading config from the agent

Previously, if the user specified a port number then this subcommand wouldn't try to load any configuration from the agent, requiring the user to provide all relevant data (e.g. authentication data, timeout lengths, etc.). If the user didn't provide a port number, then the subcommand would fetch config from the agent, look up the SNMP entry corresponding to the given IP address, and use that data, ignoring any parameters that the user provided.

After this PR, the behavior is that the subcommand will always try to load data from the agent, but any values provided by the user will overwrite those from the agent. If the agent is unreachable, the subcommand will print a warning, but will run anyway (so that it still functions even if you don't have a running agent).

This fixes NDMII-2450 and NDMII-2541

Smarter IP parsing

Previously the script assumed that if there was a : in the given IP address, then it was a host:port pair and the last segment should be parsed as a port. Now, it uses golang's built-in net.SplitHostPort, which means that it will correctly identify IPv6 addresses (which also contain colons) and other names; if an IP or hostname contains colons, it must be encased in square brackets, so e.g. to walk localhost on an explicit port you would write

agent snmp walk '[::1]:161'

Proper version/community defaults

Previously the behavior around defaults was slightly weird - if you didn't pass in a version or passed in an unrecognized version, and also didn't provide a community string, it would complain with a generic "no authentication method provided", but if you explicitly passed in version 1 or 2c then it would default to community="public".

The new behavior is

1. If you pass in an unrecognized version, it complains
2. If you don't pass in any version, but you do provide a username, it defaults to version 3
3. Otherwise, if you don't pass in any version, it defaults to version 2c
4. If it ends up using version 1 or version 2c (regardless of whether you told it to explicitly or it default to it), then a missing community string will default to "public".
5. If it ends up using version 3 and you didn't provide a username, it complains username is required for snmp v3.

Cosmetic changes and bugfixes

• When a user enters invalid input, the CLI will only show the error message and the usage string, not the full help text (which was obscuring the actual error message previously)
• Several error messages have been made clearer (in particular, if the local agent is unreachable, it will say this instead of "unexpected end of JSON input")
• If the datadog config has SNMP entries where network_address is not a valid CIDR address, this subcommand will no longer panic.
• Counters and Gauges are now formatted as e.g. Gauge32 instead of Gauge 32 in the output, to match how snmpwalk formats things
• It will no longer fail to read local config if the local config has ignored_ip_addresses set (this is issue NDMII-2541 ).

Possible Drawbacks / Trade-offs

In the specific case where the agent is reachable but very slow to provide config, it could be frustrating that an snmp walk with all necessary parameters specified nonetheless checks the config, but this is difficult to avoid without some fairly complex logic to decide how many parameters must be specified before we ignore local config (for example, if you provide all the necessary credentials etc but do not explicitly set a timeout, would you be surprised if the timeout in your config file was ignored?). It is easier to just request it every time, which in nearly cases should be fine - either the agent will reply immediately, or the connection will be refused.

Describe how to test/QA your changes

Run agent snmp walk <someaddress> against the address of a working SNMP agent (you can spin up one on localhost:161 with docker run -p 161:161/udp tandrup/snmpsim) and confirm that it generates the expected data (which should roughly match the output of snmpwalk -c public <someaddress>:<someport>).

For thoroughness, I tested it several different ways, including calling it with missing arguments:

• agent snmp walk localhost should default to SNMP v2c with the public community on port 161
• agent snmp walk localhost:162 should fail immediately because the connection is rejected
• agent snmp walk localhost -v 3 should fail immediately, complaining that you must provide a username
• agent snmp walk -u simulator -l authPriv -a MD5 -x DES -A auctoritas -X privatus -v 3 --timeout=1 -N 4c9184f37cff01bcdc32dc486ec36961 localhost should work if you're using snmpsim as described above, except I'm not sure how it generates that context name so you might need to look for the context name in the snmpsim logs and replace 4c9184f37cff01bcdc32dc486ec36961 with whatever identifier your copy is using.
• If you configure your local agent with e.g.

snmp_listeners:
  configs:
    - network_address: 127.0.0.1/32
      port: 161
      snmp_version: 3
      timeout: 1
      retries: 3
      user: simulator
      authKey: auctoritas
      authProtocol: MD5
      privKey: privatus
      privProtocol: DES
      context_name: 4c9184f37cff01bcdc32dc486ec36961

then agent snmp walk 127.0.0.1 -v3 should work (again assuming you set the right context name), and overriding any of the auth values should make it stop working (the -v3 is technically redundant but it makes sure you're not accidentally testing the default v2c instead).

[pr-commenter[bot]]

Bloop Bleep... Dogbot Here

Regression Detector Results

Run ID: f497363b-8560-4a3f-8193-e73133995572 Baseline: f986fb4b62f7374aaadf90575edf07de252b6c8b Comparison: a23b0a92ca3f2d4f7d0c3da7d666e590ee19433f

Performance changes are noted in the perf column of each table:

• ✅ = significantly better comparison variant performance
• ❌ = significantly worse comparison variant performance
• ➖ = no significant change in performance

Experiments with missing or malformed data

• basic_py_check

Usually, this warning means that there is no usable optimization goal data for that experiment, which could be a result of misconfiguration.

No significant changes in experiment optimization goals

Confidence level: 90.00% Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	+1.28	[-5.28, +7.84]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	+1.28	[-5.28, +7.84]
➖	process_agent_standard_check_with_stats	memory utilization	+0.41	[+0.38, +0.45]
➖	idle	memory utilization	+0.18	[+0.15, +0.22]
➖	trace_agent_json	ingress throughput	+0.02	[-0.02, +0.06]
➖	file_tree	memory utilization	+0.02	[-0.04, +0.07]
➖	uds_dogstatsd_to_api	ingress throughput	+0.00	[-0.00, +0.00]
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.00, +0.00]
➖	trace_agent_msgpack	ingress throughput	-0.00	[-0.01, +0.00]
➖	process_agent_real_time_mode	memory utilization	-0.02	[-0.05, +0.01]
➖	process_agent_standard_check	memory utilization	-0.16	[-0.20, -0.12]
➖	tcp_syslog_to_blackhole	ingress throughput	-0.37	[-0.43, -0.31]
➖	otel_to_otel_logs	ingress throughput	-0.72	[-1.36, -0.09]
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	-1.08	[-2.53, +0.37]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

3. Its configuration does not mark it "erratic".

[pr-commenter[bot]]

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=30146687 --os-family=ubuntu

[pr-commenter[bot]]

Regression Detector

Regression Detector Results

Run ID: 116750e3-0072-4e0e-9225-7eacae9e3a44 Baseline: 4f31cd37d48631f4e72d50922f22731cd8d06615 Comparison: ed222479fef6a0ab94474fb8205ba1ae14c780a1

Performance changes are noted in the perf column of each table:

• ✅ = significantly better comparison variant performance
• ❌ = significantly worse comparison variant performance
• ➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00% Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	-1.84	[-8.16, +4.49]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_tree	memory utilization	+2.01	[+1.90, +2.13]
➖	idle	memory utilization	+0.51	[+0.47, +0.55]
➖	process_agent_standard_check_with_stats	memory utilization	+0.43	[+0.39, +0.47]
➖	process_agent_real_time_mode	memory utilization	+0.25	[+0.22, +0.29]
➖	process_agent_standard_check	memory utilization	+0.17	[+0.13, +0.22]
➖	trace_agent_json	ingress throughput	+0.03	[-0.00, +0.06]
➖	trace_agent_msgpack	ingress throughput	-0.01	[-0.02, -0.00]
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.01	[-0.04, +0.01]
➖	uds_dogstatsd_to_api	ingress throughput	-0.02	[-0.22, +0.19]
➖	tcp_syslog_to_blackhole	ingress throughput	-0.09	[-0.18, -0.00]
➖	otel_to_otel_logs	ingress throughput	-0.11	[-0.56, +0.34]
➖	pycheck_1000_100byte_tags	% cpu utilization	-0.36	[-5.30, +4.59]
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	-1.51	[-4.19, +1.18]
➖	file_to_blackhole	% cpu utilization	-1.84	[-8.16, +4.49]
➖	basic_py_check	% cpu utilization	-1.85	[-4.47, +0.78]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

3. Its configuration does not mark it "erratic".

[dplepage-dd]

/merge

[dd-devflow[bot]]

:steam_locomotive: MergeQueue

Pull request added to the queue.

This build is next! (estimated merge in less than 29m)

Use /merge -c to cancel this operation!

[dd-devflow[bot]]

:rotating_light: MergeQueue

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

If you need support, contact us on Slack #ci-interfaces with those details!

[paulcacheux]

/merge

[dd-devflow[bot]]

:steam_locomotive: MergeQueue

Pull request added to the queue.

There are 3 builds ahead! (estimated merge in less than 29m)

Use /merge -c to cancel this operation!