Componentize the snmp subcommands #29148
Conversation
Go Package Import DifferencesBaseline: b83d36e
|
|
Great work on creating the new component 🎉 👏 Left a few suggestions. Also, would be great if we could add test for the new component 😄 |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: b83d36e Regression Detector: ✅ Bounds Checks: ✅ No significant changes in experiment optimization goalsConfidence level: 90.00% There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | basic_py_check | % cpu utilization | +3.38 | [-0.70, +7.46] | 1 | Logs |
| ➖ | idle_all_features | memory utilization | +0.55 | [+0.46, +0.65] | 1 | Logs |
| ➖ | pycheck_lots_of_tags | % cpu utilization | +0.45 | [-3.21, +4.11] | 1 | Logs |
| ➖ | idle | memory utilization | +0.23 | [+0.18, +0.27] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | +0.20 | [-0.29, +0.70] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +0.14 | [-0.59, +0.87] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | +0.05 | [-0.14, +0.23] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.01 | [-0.21, +0.24] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.01, +0.01] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.00 | [-0.10, +0.10] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | -0.02 | [-0.36, +0.32] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | -0.10 | [-0.35, +0.15] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.23 | [-0.35, -0.12] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.24 | [-0.28, -0.19] | 1 | Logs |
| ➖ | otel_to_otel_logs | ingress throughput | -1.28 | [-2.08, -0.48] | 1 | Logs |
Bounds Checks Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | idle | memory_usage | 10/10 |
Explanation
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
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:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
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.
-
Its configuration does not mark it "erratic".
FlorianVeaux
force-pushed
the
flo/snmpcommand-component
branch
2 times, most recently
from
1b6062c
to
626210a
Compare
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=46279751 --os-family=ubuntuNote: This applies to commit c77f15e |
| err = snmpScanner.Comp.RunDeviceScan(snmpConnection, "default") | ||
| assert.ErrorContains(t, err, "&GoSNMP.Conn is missing. Provide a connection or use Connect()") | ||
|
|
||
| err = snmpScanner.Comp.RunDeviceScan(snmpConnection, "default") | ||
| assert.ErrorContains(t, err, "&GoSNMP.Conn is missing. Provide a connection or use Connect()") |
There was a problem hiding this comment.
Is there a way to add more complete test coverage? I understand that it might be difficult as we need snmp connection. Is there any way to stub it or something like it?
There was a problem hiding this comment.
This test will also fail if gosnmp ever changes their error text, which isn't great, but I guess if GoSNMP isn't returning typed errors there's not a lot we can do about that.
There was a problem hiding this comment.
A lot of code is taking a *gosnmp.GoSNMP struct instance as a param which makes it really hard for stubbing without a large refactor of the snmp codebase at the moment.
We could build a more integration oriented test but that's also a very large effort that we'd rather not do at the current moment (mocking an snmp server is not easy). Although as part of our effort to build more e2e tests this is one of our good candidates.
So if you don't mind I'd like to leave things as is, the code that this PR is introducing is tested via the current file, although the existing code wasn't tested and still isn't tested after this PR.
FlorianVeaux
force-pushed
the
flo/snmpcommand-component
branch
from
0e5d13f
to
3305587
Compare
| forwarder, err := s.demux.GetEventPlatformForwarder() | ||
| if err != nil { | ||
| return fmt.Errorf("unable to get sender: %w", err) | ||
| } |
There was a problem hiding this comment.
We could move this code to the component initialize function and return an error the event platform is not properly initialized. That would catch issue earlier, and it would avoid having to call this function everytime we execute the device scan command.
func NewComponent(reqs Requires) (Provides, error) {
forwarder, err := s.demux.GetEventPlatformForwarder()
if err != nil {
return Provides{}, err
}
scanner := snmpScannerImpl{
log: reqs.Logger,
forwarder: forwarder,
}
provides := Provides{
Comp: scanner,
}
return provides, nil
}
Componentize the "walk" and "scan" SNMP commands. This is to allow registering a new "AgentTaskListener" with remote-config that will later call the RunDeviceScan method of the component