Enable K6 by default in agent deployments #722

The-9880 · 2024-06-06T13:52:23Z

Summary:

Removes --features from the cmd, adds an explicit --disable-k6 to disable the runner + k6 checks.
Updates the error message for k6 capability missing on probe.

Users upgrading from older probes (will) have k6 scripts disabled by default in the API if they weren't already running k6 checks. There are 2 cases for them:

Had k6 FF in args: no impact, this setup makes it default-enabled.
Did not have k6 FF: this was the default. If they use the Docker image or have k6 on their PATH, then their upgraded agent would be able to set up a k6Runner. Any previously-assigned MultiHTTP or scripted checks may start running. They will need to enable in the UI to assign k6 checks.

For new probes, this change aligns the agent's default with the default assumption of the UI/Terraform (k6-enabled), so it should reduce issues with setup.

Also updated the error message when k6 is missing - feedback appreciated.

The-9880 · 2024-06-07T14:07:08Z

internal/checks/checks.go

@@ -53,7 +53,7 @@ func (e FatalError) Error() string { return string(e) }
 var _ error = FatalError("")

 const (
-	errCapabilityK6Missing = FatalError("k6 is required for scripted check support")
+	errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")


Suggested change

errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")

errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or disable the probe's k6 capability in the SM app")

Thoughts on this rewording? I can't find a good, succinct way to describe this.

What comes to my mind is

Suggested change

errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")

errCapabilityK6Missing = FatalError("synthetic monitoring backend requested k6 to be enabled on this probe, but it is disabled by CLI arg")

roobre · 2024-06-07T14:52:04Z

internal/checks/checks.go

@@ -53,7 +53,7 @@ func (e FatalError) Error() string { return string(e) }
 var _ error = FatalError("")

 const (
-	errCapabilityK6Missing = FatalError("k6 is required for scripted check support")
+	errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")


What comes to my mind is

Suggested change

errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")

errCapabilityK6Missing = FatalError("synthetic monitoring backend requested k6 to be enabled on this probe, but it is disabled by CLI arg")

roobre · 2024-06-07T14:55:43Z

cmd/synthetic-monitoring-agent/main.go

+	if !config.DisableK6 {
+		if err := features.Set(feature.K6); err != nil {
+			return fmt.Errorf("cannot set k6 feature: %w", err)
+		}
+	}


It's not fully clear to me how the new flag (--disable-k6) and the old one (--features=k6) interact.

I think it makes sense having a dedicated K6 flag for disabling k6, but that makes me wonder if we should get rid of, or deprecate the feature flag.

This change does get rid of the --features flag on L105-106. I could "formally" deprecate it by detecting it and printing out a message.

How the new vs old flags interact
features is a collection object which holds active features enabled on this probe. It used to have traceroute and adhoc when those were FFs, but now only k6 remains as functionality that can be disabled.

The old flag --features was captured by flags.Var, which implicitly called features.Set on the arg value. features.Set assumes a csv input and splits it to save each element.

Example: features.Set("traceroute,k6,adhoc") -> map[string]struct{}{"traceroute": struct{}, "k6": struct{}, "adhoc": struct{}}.

With this change, --features is gone. We explicitly call features.Set("k6") unless --disable-k6 is set, making k6 a default-enabled feature on the probe.

Ah, you're definitely right, I did not see the removal of the --features CLI flag 🙈
Looking good :)

* Chore(deps): Bump golang.org/x/net from 0.24.0 to 0.25.0 * Chore(deps): Bump the prometheus-go group across 1 directory with 2 updates * Chore(deps): Bump github.com/mccutchen/go-httpbin/v2 * Update to grafana-build-tools v0.11.0 (#705) * Remove adhoc and traceroute feature flags. (#707) * Chore(deps): Bump github.com/KimMachineGun/automemlimit * checks/test: make timer big enough for context cancel to be picked up * Fix: Interpolate variables into MultiHTTP request bodies (#713) * Chore(deps): Bump github.com/prometheus/prometheus (#716) * Chore(deps): Bump github.com/rs/zerolog from 1.32.0 to 1.33.0 * --- updated-dependencies: - dependency-name: kernel.org/pub/linux/libs/security/libcap/cap dependency-type: direct:production update-type: version-update:semver-patch ... * MultiHttp script should decode the payload as a string and replace the variable placeholders with the values. The result should be assigned to the request body. (#717) * Build(deps): Bump golang.org/x/net from 0.25.0 to 0.26.0 * Enable K6 by default in agent deployments (#722) Signed-off-by: Anant Sharma <anant.sharma@grafana.com>

* Chore(deps): Bump golang.org/x/net from 0.24.0 to 0.25.0 * Chore(deps): Bump the prometheus-go group across 1 directory with 2 updates * Chore(deps): Bump github.com/mccutchen/go-httpbin/v2 * Update to grafana-build-tools v0.11.0 (#705) * Remove adhoc and traceroute feature flags. (#707) * Chore(deps): Bump github.com/KimMachineGun/automemlimit * checks/test: make timer big enough for context cancel to be picked up * Fix: Interpolate variables into MultiHTTP request bodies (#713) * Chore(deps): Bump github.com/prometheus/prometheus (#716) * Chore(deps): Bump github.com/rs/zerolog from 1.32.0 to 1.33.0 * --- updated-dependencies: - dependency-name: kernel.org/pub/linux/libs/security/libcap/cap dependency-type: direct:production update-type: version-update:semver-patch ... * MultiHttp script should decode the payload as a string and replace the variable placeholders with the values. The result should be assigned to the request body. (#717) * Build(deps): Bump golang.org/x/net from 0.25.0 to 0.26.0 * Enable K6 by default in agent deployments (#722) * Fix: deprecate --features and warn user (#726) * k6runner: use check context for http request (#715) Signed-off-by: Anant Sharma <anant.sharma@grafana.com>

The-9880 added 2 commits June 6, 2024 09:31

enabling K6 by default in agent deployments.

51794d0

Update error message for missing k6 capability.

eb8e891

The-9880 force-pushed the default-k6 branch from 888219c to eb8e891 Compare June 6, 2024 14:00

The-9880 commented Jun 7, 2024

View reviewed changes

The-9880 marked this pull request as ready for review June 7, 2024 14:19

The-9880 requested a review from a team as a code owner June 7, 2024 14:19

roobre reviewed Jun 7, 2024

View reviewed changes

roobre approved these changes Jun 7, 2024

View reviewed changes

The-9880 merged commit 123b0d6 into main Jun 10, 2024
4 checks passed

The-9880 deleted the default-k6 branch June 10, 2024 13:39

The-9880 mentioned this pull request Jun 10, 2024

Release v0.24.2 #725

Closed

The-9880 mentioned this pull request Jun 11, 2024

Release v0.24.2 #728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable K6 by default in agent deployments #722

Enable K6 by default in agent deployments #722

The-9880 commented Jun 6, 2024 •

edited

Loading

The-9880 Jun 7, 2024 •

edited

Loading

roobre Jun 7, 2024

roobre Jun 7, 2024

roobre Jun 7, 2024

The-9880 Jun 7, 2024 •

edited

Loading

roobre Jun 7, 2024

	errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")
	errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or disable the probe's k6 capability in the SM app")

	errCapabilityK6Missing = FatalError("k6 is required for scripted check support - configure k6 or edit probe capabilities in the SM app")
	errCapabilityK6Missing = FatalError("synthetic monitoring backend requested k6 to be enabled on this probe, but it is disabled by CLI arg")

Enable K6 by default in agent deployments #722

Enable K6 by default in agent deployments #722

Conversation

The-9880 commented Jun 6, 2024 • edited Loading

The-9880 Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

roobre Jun 7, 2024

Choose a reason for hiding this comment

roobre Jun 7, 2024

Choose a reason for hiding this comment

roobre Jun 7, 2024

Choose a reason for hiding this comment

The-9880 Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

roobre Jun 7, 2024

Choose a reason for hiding this comment

The-9880 commented Jun 6, 2024 •

edited

Loading

The-9880 Jun 7, 2024 •

edited

Loading

The-9880 Jun 7, 2024 •

edited

Loading