[ASCII-1513] Add json flag to diagnose command #25010
Conversation
louis-cqrl
added
team/agent-shared-components
and removed
team/agent-shared-components
labels
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=32818544 --os-family=ubuntu |
| StatusCode: 400, // Hardcoded 400 status code since error is not nil | ||
| URL: url, |
There was a problem hiding this comment.
Are these necessary and sure we don't change an existing behaviour?
There was a problem hiding this comment.
they are new field of diagnosis, they are not mandatory so it should not change anything
| }) | ||
| } else { | ||
| url, err := logshttp.CheckConnectivityDiagnose(endpoints.Main, config.Datadog) | ||
|
|
||
| name := fmt.Sprintf("Connectivity to %s", url) | ||
| diag := createDiagnosis(name, url, "", err) | ||
| diag := createDiagnosis("", 200, name, url, "", err) // Hardcoded 200 status code since error is nil |
There was a problem hiding this comment.
Don't we have any endpoint name to set here?
There was a problem hiding this comment.
the names seems to be the url so it is not needed
Regression DetectorRegression Detector ResultsRun ID: b6b57bcd-0ba1-4d0b-afb2-7192434dbebd 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 |
|---|---|---|---|---|
| ✅ | file_to_blackhole | % cpu utilization | -19.37 | [-24.45, -14.30] |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI |
|---|---|---|---|---|
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +10.87 | [-11.42, +33.17] |
| ➖ | pycheck_1000_100byte_tags | % cpu utilization | +0.79 | [-3.86, +5.44] |
| ➖ | process_agent_standard_check_with_stats | memory utilization | +0.54 | [+0.48, +0.60] |
| ➖ | basic_py_check | % cpu utilization | +0.41 | [-2.11, +2.92] |
| ➖ | trace_agent_msgpack | ingress throughput | +0.01 | [-0.00, +0.02] |
| ➖ | trace_agent_json | ingress throughput | +0.00 | [-0.01, +0.01] |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.05, +0.04] |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.01 | [-0.21, +0.19] |
| ➖ | idle | memory utilization | -0.14 | [-0.17, -0.11] |
| ➖ | otel_to_otel_logs | ingress throughput | -0.21 | [-0.59, +0.16] |
| ➖ | process_agent_real_time_mode | memory utilization | -0.22 | [-0.27, -0.18] |
| ➖ | process_agent_standard_check | memory utilization | -0.32 | [-0.38, -0.26] |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | -0.98 | [-3.84, +1.88] |
| ✅ | file_to_blackhole | % cpu utilization | -19.37 | [-24.45, -14.30] |
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".
Co-authored-by: Ursula Chen <58821586+urseberry@users.noreply.github.com>
Co-authored-by: Ursula Chen <58821586+urseberry@users.noreply.github.com>
There was a problem hiding this comment.
The JSON part LGTM but I'm not sure about the fields you added tbh. If technically possible I would prefer these to be hidden behind an interface (so we don't need to add new fields for new type of diagnose)
@iglendd can we have your thoughts on this please since you designed that part 🙏
| @@ -35,6 +35,7 @@ Flags: | |||
| -t, --list list diagnose suites | |||
| -l, --local force diagnose execution by the command line instead of the agent process (useful when troubleshooting privilege related problems) | |||
| -v, --verbose verbose output, includes passed diagnoses, and diagnoses description | |||
| -j, --json output in a json file | |||
There was a problem hiding this comment.
❓ question: you mean format here (instead of file) ?
There was a problem hiding this comment.
doc string from the code is output diagnosis results in JSON format. We should use the same for the doc,
There was a problem hiding this comment.
hoooo yes good catch I'll modify it this evening
| Normally, internal diagnose functions run in the context of the Agent and other services. It can be overridden with –run-as-user options, and if specified, diagnose functions are executed in the context of the Agent diagnose CLI process if possible. | ||
|
|
||
| ## ```json``` option | ||
| Normally, diagnose is displayed on stdout. If the JSON option is specified, the output is formatted as JSON |
There was a problem hiding this comment.
💭 thought: JSON is still printed on stdout right ? If yes, I don't think the first sentence is needed.
There was a problem hiding this comment.
yes it changed at the last minute to just display it on stdout
pkg/diagnose/runner.go
Outdated
|
|
||
| diagnose, err := runDiagnose(w, diagCfg, deps) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
❓ question: are we sure that err is JSON formatted here ? (e.g. if I have an error when I run --json I might want a JSON output {"error": "..."})
There was a problem hiding this comment.
No this is not json formatted but I can do it
There was a problem hiding this comment.
I tried something but not sure it will work I let you check
I added it in the field of diagnosis cause it seems difficult to take back the informations through all different functions it goes |
| @@ -35,6 +35,7 @@ Flags: | |||
| -t, --list list diagnose suites | |||
| -l, --local force diagnose execution by the command line instead of the agent process (useful when troubleshooting privilege related problems) | |||
| -v, --verbose verbose output, includes passed diagnoses, and diagnoses description | |||
| -j, --json output in a json file | |||
There was a problem hiding this comment.
doc string from the code is output diagnosis results in JSON format. We should use the same for the doc,
| Name: name, | ||
| StatusCode: statusCode, | ||
| URL: logURL, | ||
| EndpointName: endpointName, |
There was a problem hiding this comment.
What's the different between Name and EndpointName ?
There was a problem hiding this comment.
Most of the time name is in this format:
"Connectivity to <endpoint url> (is Ok|failed)"
and endpointName is the name of the endpoint such as series_v2 flare etc
releasenotes/notes/add-json-outpout-to-diagnose-command-ed472034cd227e23.yaml
Outdated
Show resolved
Hide resolved
pkg/diagnose/diagnosis/loader.go
Outdated
| // endpoint URL | ||
| URL string | ||
| // Name of the endpoint | ||
| EndpointName string |
There was a problem hiding this comment.
If we already have a Name field, why do we need a EndpointName ? Also diagnose could be about anything, not just endpoint so we should avoid any specific field like URL, StatusCode or EndpointName.
In general, the --json option should show the exact same data than the regular version of the command but in a JSON format.
There was a problem hiding this comment.
If we want to use the field name we will need to refacto a bit diagnose code, since as I talked above name field is just uses to store a whole string that does not fit to what we want to display
…34cd227e23.yaml Co-authored-by: maxime mouial <hush-hush@users.noreply.github.com>
…tadata.go, loader.go, and runner.go
Co-authored-by: GustavoCaso <gustavo.caso@datadoghq.com>
* Remove SuiteDeps from ListStdOut * Remove diagnose.Run in api component (Use either CLI or InProcess version) * As `RunningInAgentProcess` is false, calling `getSuitesForCLIProcess` is equivalent to calling `getSuites` * Simplify cluster agent diagnose command Create RunStdOutAutodiscovery that runs only check "connectivity-datadog-autodiscovery" * Remove diagnose.SuitesDeps from GetClusterAgentDiagnose * Fix cmdDiagnose * Remove `RunningInAgentProcess` * Remove getDiagnose * Inline some functions * Add comment to improve API component * Replace RunStdOutAutodiscovery by RunStdOutLocalCheck * Improve code after merge from origin/main
Go Package Import DifferencesBaseline: d73006c
|
What does this PR do?
This PR allow diagnose output in json format
Motivation
https://datadoghq.atlassian.net/browse/ASCII-325
Additional Notes
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Run
datadog-agent diagnose -janddatadog-agent diagnose --jsonSee if the output is a valid JSON
See if it contains the same information as
datadog-agent diagnose -v