[ASCII-1025] Render check output using status component #21881
Conversation
GustavoCaso
force-pushed
the
render-check-output-using-status-component
branch
from
c6cf8af to
89b001f
Compare
Go Package Import DifferencesBaseline: b8ceee3
|
GustavoCaso
changed the title
GustavoCaso
force-pushed
the
render-check-output-using-status-component
branch
from
6b52b7d to
7d485d9
Compare
Bloop Bleep... Dogbot HereRegression Detector ResultsRun ID: c544acb2-412a-44f6-8cfd-a25295d7f042 Performance changes are noted in the perf column of each table:
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 |
|---|---|---|---|---|
| ➖ | idle | memory utilization | +0.44 | [+0.42, +0.47] |
| ➖ | file_to_blackhole | % cpu utilization | -0.03 | [-6.58, +6.51] |
| ➖ | file_tree | memory utilization | -0.48 | [-0.58, -0.39] |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI |
|---|---|---|---|---|
| ➖ | otel_to_otel_logs | ingress throughput | +0.57 | [-0.12, +1.26] |
| ➖ | idle | memory utilization | +0.44 | [+0.42, +0.47] |
| ➖ | process_agent_real_time_mode | memory utilization | +0.18 | [+0.14, +0.21] |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.06, +0.06] |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.00 | [-0.03, +0.03] |
| ➖ | process_agent_standard_check | memory utilization | -0.00 | [-0.08, +0.07] |
| ➖ | trace_agent_msgpack | ingress throughput | -0.02 | [-0.04, -0.00] |
| ➖ | file_to_blackhole | % cpu utilization | -0.03 | [-6.58, +6.51] |
| ➖ | trace_agent_json | ingress throughput | -0.05 | [-0.08, -0.02] |
| ➖ | process_agent_standard_check_with_stats | memory utilization | -0.06 | [-0.13, +0.00] |
| ➖ | file_tree | memory utilization | -0.48 | [-0.58, -0.39] |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.81 | [-0.87, -0.74] |
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:
-
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".
| "lastErrorTraceback": func(s string) htemplate.HTML { return doNotEscape(lastErrorTraceback(s)) }, | ||
| "lastErrorMessage": func(s string) htemplate.HTML { return doNotEscape(lastErrorMessage(s)) }, |
There was a problem hiding this comment.
Those two functions used to call doNotEscape, I don't think it's called in the new version ?
There was a problem hiding this comment.
Those functions were previously overwritten by the GUI (The only user of these functions)
So, I replaced the functions with those ones. Also, the new functions effectively call the same functionality as doNotEscape
There was a problem hiding this comment.
There is room for cleanup and improvement. Which I will do on subsequent PRs
comp/core/status/render_helpers.go
Outdated
| switch v := unixTime.(type) { | ||
| case int64: | ||
| return parseFunction(int64(v)) | ||
| case float64: | ||
| return parseFunction(int64(v)) | ||
| default: | ||
| return "" |
There was a problem hiding this comment.
💭 thought: Instead of defining a parse function, you could just perform the cast at the start of the function
There was a problem hiding this comment.
I don't mind having the parsing function
pkg/cli/subcommands/check/command.go
Outdated
| checkMap := make(map[checkid.ID]*stats.Stats) | ||
| checkMap[c.ID()] = s | ||
| var checkResult map[string]interface{} | ||
| resultBytes, err := json.Marshal(s) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = json.Unmarshal(resultBytes, &checkResult) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| checkMap := make(map[string]interface{}) | ||
|
|
||
| checkMap[string(c.ID())] = checkResult |
There was a problem hiding this comment.
❓ question: Why do you need to marshal and unmarshal instead of just adding the check stat in the map like before ?
There was a problem hiding this comment.
Previously marshall was happening here: https://github.com/DataDog/datadog-agent/pull/21881/files#diff-4a83275fbbf7764c4d251dde368ca784c146ed15c8b49dbc9946ecb0450e57b2L513 and the unmarshal was happening here:
datadog-agent/pkg/status/render/render.go
Line 207 in 7d485d9
We need to transform *stats.Stats to a map[string]interface{} because the helper functions expect to have that signature:
datadog-agent/comp/core/status/render_helpers.go
Lines 282 to 299 in 6c7bb55
So we just moved the marshal and unmarshal to a different location.
There was a problem hiding this comment.
Mind adding instructions for testing the process agent status? Similar to your previous PR: #20824
…r function for UnixTime
GustavoCaso
force-pushed
the
render-check-output-using-status-component
branch
from
ff32f9e to
e0ac5ce
Compare
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is next! (estimated merge in less than 44m) you can cancel this operation by commenting your pull request with |
dd-devflow
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
dd-mergequeue
bot
deleted the
render-check-output-using-status-component
branch
dd-devflow
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What does this PR do?
Use the
comp/core/statuscomponent to collect and render the output for theagent checksubcommand.We are slowly introducing the status component as the main component to handle the status for all agent flavors.
This PR changes:
pkg/statushelper functions.pkg/cli/subcommands/check/command.goto use the status componentMotivation
Additional Notes
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Executing the
check <checkName>command should work as expected.Steps to reproduce it:
./bin/agent/agent check cpu -c dev/dist/datadog.yaml. Validate that the output is correct../bin/agent/agent check cpu --json -c dev/dist/datadog.yaml. Validate that the output is correctAgent GUI
./bin/agent/agent launch-gui -c dev/dist/datadog.yaml.Process agent
/opt/datadog-agent/embedded/bin/process-agent status. Validate that the output is correct.Reviewer's Checklist
Triagemilestone is set.major_changelabel if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.changelog/no-changeloglabel has been applied.qa/skip-qalabel, with required eitherqa/doneorqa/no-code-changelabels, are applied.team/..label has been applied, indicating the team(s) that should QA this change.need-change/operatorandneed-change/helmlabels have been applied.k8s/<min-version>label, indicating the lowest Kubernetes version compatible with this feature.