Fix fx related issues for agenttelemetry improvements #29789

GustavoCaso · 2024-10-04T09:16:31Z

What does this PR do?

Fix the Fx related issues and linter. make sure the agenttelemetry follow the latest component framework guidance. Add a test for the new command
Make the new command follow the same pattern as the other to show display metadata. agent diagnose show-metadata agent-telemetrty
Ensure that if the agent telemetry feature is disable, calling the command agent diagnose show-metadata agent-telemetrty do not panic
Fix a few linters issues

Motivation

Help @iglendd get the agenttelemetry improvements PR merged 😄

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

…tions. Ensure we do panic when returning the agent-telemetry payload if agent telemetry is disable

GustavoCaso · 2024-10-04T09:17:26Z

comp/core/agenttelemetry/impl/agenttelemetry.go

@@ -376,6 +371,11 @@ func (a *atel) run(profiles []*Profile) {
 }

 func (a *atel) writePayload(w http.ResponseWriter, _ *http.Request) {
+	if !a.enabled {


We need to check id the feature is enable otherwise a.logComp would panic 😄

GustavoCaso · 2024-10-04T09:17:58Z

comp/core/agenttelemetry/impl/agenttelemetry.go

@@ -376,6 +371,11 @@ func (a *atel) run(profiles []*Profile) {
 }

 func (a *atel) writePayload(w http.ResponseWriter, _ *http.Request) {
+	if !a.enabled {
+		httputils.SetJSONError(w, errors.New("agent-telemetry is not enabled. Please enable agent telemetry"), 400)


@iglendd I'm open to change the error message display to the users, maybe a link to the documentation would be nice

cit-pr-commenter · 2024-10-04T10:15:42Z

Regression Detector

Regression Detector Results

Run ID: add3d311-39e2-49ec-95b2-bb512c6b2384 Metrics dashboard Target profiles

Baseline: 596053e
Comparison: 0f63b8a

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.

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+1.51	[+0.77, +2.24]	1	Logs
➖	otel_to_otel_logs	ingress throughput	+0.87	[+0.06, +1.68]	1	Logs
➖	tcp_syslog_to_blackhole	ingress throughput	+0.48	[+0.42, +0.53]	1	Logs
➖	pycheck_lots_of_tags	% cpu utilization	+0.42	[-2.06, +2.90]	1	Logs
➖	file_tree	memory utilization	+0.39	[+0.29, +0.49]	1	Logs
➖	basic_py_check	% cpu utilization	+0.24	[-2.46, +2.95]	1	Logs
➖	idle	memory utilization	+0.14	[+0.10, +0.19]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	+0.00	[-0.11, +0.11]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.01, +0.01]	1	Logs
➖	idle_all_features	memory utilization	-0.31	[-0.40, -0.22]	1	Logs

Bounds Checks

perf	experiment	bounds_check_name	replicates_passed
✅	idle	memory_usage	10/10

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".

pr-commenter · 2024-10-04T11:25:36Z

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=46597670 --os-family=ubuntu

Note: This applies to commit e482bf1

misteriaud · 2024-10-14T07:46:10Z

cmd/agent/subcommands/diagnose/command_test.go

+		[]string{"diagnose", "show-metadata", "agent-telemetry"},
+		printPayload,
+		func(_ core.BundleParams, secretParams secrets.Params) {
+			require.Equal(t, false, secretParams.Enabled)


❓ question: ‏What is the goal of this test ?

I would expect this test to check "agent-telemetry"

@ogaca-dd @misteriaud fixed 😄

ogaca-dd · 2024-10-15T08:44:19Z

cmd/agent/subcommands/diagnose/command.go

 	showAgentTelemetryCommand := &cobra.Command{
-		Use:   "show-telemetry",
+		Use:   "agent-telemetry",


Note: This is a breaking change. I guess this should be OK as the command is very new.

It is not a breaking change as the command is not release yet. This PR uses this PR as the starting point #29770

The starting point PR hasn't being merge yet

ogaca-dd · 2024-10-15T08:46:21Z

cmd/agent/subcommands/diagnose/command_test.go

+		[]string{"diagnose", "show-metadata", "agent-telemetry"},
+		printPayload,
+		func(_ core.BundleParams, secretParams secrets.Params) {
+			require.Equal(t, false, secretParams.Enabled)


I would expect this test to check "agent-telemetry"

GustavoCaso · 2024-11-07T15:08:57Z

/merge

dd-devflow · 2024-11-07T15:09:12Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-11-07 15:09:11 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in len.gamburg/agent-telemetry-improve is 0s.

GustavoCaso added 2 commits October 4, 2024 10:44

Update agenttelemetry to follow the latest component framework conven…

4b0e5b7

…tions. Ensure we do panic when returning the agent-telemetry payload if agent telemetry is disable

fix fx linters about no testing the new diagnose command

320605d

GustavoCaso requested review from a team as code owners October 4, 2024 09:16

GustavoCaso commented Oct 4, 2024

View reviewed changes

fix minor linter issues

0f63b8a

GustavoCaso force-pushed the fix-fx-related-issues-for-agenttelemetry-improvements branch from 12e7629 to 0f63b8a Compare October 4, 2024 10:54

misteriaud reviewed Oct 14, 2024

View reviewed changes

ogaca-dd reviewed Oct 15, 2024

View reviewed changes

ensure agent telemetry command test the right parameter

e482bf1

ogaca-dd approved these changes Oct 17, 2024

View reviewed changes

dd-mergequeue bot merged commit 3501a68 into len.gamburg/agent-telemetry-improve Nov 7, 2024
218 checks passed

dd-mergequeue bot deleted the fix-fx-related-issues-for-agenttelemetry-improvements branch November 7, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fx related issues for agenttelemetry improvements #29789

Fix fx related issues for agenttelemetry improvements #29789

GustavoCaso commented Oct 4, 2024 •

edited

Loading

GustavoCaso Oct 4, 2024

GustavoCaso Oct 4, 2024

cit-pr-commenter bot commented Oct 4, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

pr-commenter bot commented Oct 4, 2024 •

edited

Loading

misteriaud Oct 14, 2024

ogaca-dd Oct 15, 2024

GustavoCaso Oct 15, 2024

ogaca-dd Oct 15, 2024

GustavoCaso Oct 15, 2024 •

edited

Loading

ogaca-dd Oct 15, 2024

GustavoCaso commented Nov 7, 2024

dd-devflow bot commented Nov 7, 2024 •

edited

Loading

Fix fx related issues for agenttelemetry improvements #29789

Fix fx related issues for agenttelemetry improvements #29789

Conversation

GustavoCaso commented Oct 4, 2024 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

GustavoCaso Oct 4, 2024

Choose a reason for hiding this comment

GustavoCaso Oct 4, 2024

Choose a reason for hiding this comment

cit-pr-commenter bot commented Oct 4, 2024 • edited Loading

Regression Detector

Regression Detector Results

No significant changes in experiment optimization goals

Fine details of change detection per experiment

Bounds Checks

Explanation

pr-commenter bot commented Oct 4, 2024 • edited Loading

Test changes on VM

misteriaud Oct 14, 2024

Choose a reason for hiding this comment

ogaca-dd Oct 15, 2024

Choose a reason for hiding this comment

GustavoCaso Oct 15, 2024

Choose a reason for hiding this comment

ogaca-dd Oct 15, 2024

Choose a reason for hiding this comment

GustavoCaso Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

ogaca-dd Oct 15, 2024

Choose a reason for hiding this comment

GustavoCaso commented Nov 7, 2024

dd-devflow bot commented Nov 7, 2024 • edited Loading

Devflow running: /merge

GustavoCaso commented Oct 4, 2024 •

edited

Loading

cit-pr-commenter bot commented Oct 4, 2024 •

edited

Loading

pr-commenter bot commented Oct 4, 2024 •

edited

Loading

GustavoCaso Oct 15, 2024 •

edited

Loading

dd-devflow bot commented Nov 7, 2024 •

edited

Loading

Devflow running: `/merge`