[NPM-3754] Use remote hostname from core agent #34911
Conversation
github-actions
bot
added
team/agent-runtimes
team/networks
short review
pimlu
added
changelog/no-changelog
qa/done
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: dda inv aws.create-vm --pipeline-id=58611770 --os-family=ubuntuNote: This applies to commit 324bed5 |
Uncompressed package size comparisonComparison with ancestor Size reduction summary
Diff per package
Decision✅ Passed |
Static quality checks ✅Please find below the results from static quality gates Successful checksInfo
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: cf6b9b7 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +1.35 | [-1.43, +4.13] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +0.79 | [+0.73, +0.86] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | +0.72 | [+0.68, +0.77] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | +0.26 | [+0.20, +0.32] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | +0.13 | [-0.33, +0.59] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http2 | egress throughput | +0.06 | [-0.79, +0.90] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.05 | [-0.73, +0.83] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.04 | [-0.65, +0.73] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.00 | [-0.30, +0.30] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.03, +0.03] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.01 | [-0.07, +0.05] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_0ms_latency_http1 | egress throughput | -0.01 | [-0.88, +0.86] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | -0.01 | [-0.65, +0.62] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | -0.03 | [-0.86, +0.80] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.12 | [-0.90, +0.65] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.13 | [-0.26, -0.01] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | -1.22 | [-2.05, -0.39] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | |
| ✅ | quality_gate_logs | memory_usage | 10/10 |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
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".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
| @@ -52,7 +52,8 @@ type remotehostimpl struct { | |||
| cache *cache.Cache | |||
| } | |||
|
|
|||
| func newRemoteHostImpl() hostnameinterface.Component { | |||
| // NewRemoteHostImpl returns a new hostname.Component which queries the core agent remotely | |||
| func NewRemoteHostImpl() hostnameinterface.Component { | |||
There was a problem hiding this comment.
Note I changed this to public, similar to the existing NewHostnameService() component. It seems fx has already run by the time the traceroute module is being initialized, so I couldn't figure out how to provide this with fx. If there is a smarter way, I would be interested to hear haha
There was a problem hiding this comment.
Ah I didn't see this comment before writing mine 🤔
There was a problem hiding this comment.
But still adding the hostname comp to the dependencies and passing it down to Runner should work ?
There was a problem hiding this comment.
Unfortunately this doesn't live inside the NP collector (which is in process-agent) - the Runner is in system-probe, in the traceroute module. The issue I ran into is that all system-probe modules use the same dependency struct, and I didn't want to add a dependency that the other system-probe modules don't use.
There was a problem hiding this comment.
Ah yep I didn't notice this was called from system-rpobe too...
Is that the only system-probe module using the hostname, or just the only one using the component ?
I'm guessing other modules use pkg/util/hostname directly, so having the hostname comp in the dependencies is probably better as it will allow those to use the component eventually
What do you think ?
There was a problem hiding this comment.
Yeah that's the only module using it right now. It does seem fair to expect other modules to want the hostname in the future though, so I'll add it to the struct
There was a problem hiding this comment.
I did most of the fx changes in a separate refactor: [sysprobe] Pass factory deps directly, add hostname.Component
Now this PR is really small haha
github-actions
bot
added
medium review
pimlu
changed the title
pimlu
force-pushed
the
sgeipel/remote-hostname
branch
from
e041bc8 to
7ccdd1b
Compare
github-actions
bot
added
long review
pimlu
force-pushed
the
sgeipel/remote-hostname
branch
from
95ea573 to
8e02249
Compare
|
Removing Container Intake as reviewers as we are not codeowners of any modified files |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What does this PR do?
Makes the traceroute module in system-probe query the core agent for the hostname
Motivation
Fixes a bug where the hostnames got out of sync
Describe how you validated your changes
Before:
/traceroute500s because it can't find the hostname on anagent-sandboxEKS cluster with default settingsAfter: Network Path works on the same cluster
Possible Drawbacks / Trade-offs
Additional Notes