From 27a02e928b38d9555fd99a2af70727f8d6c14625 Mon Sep 17 00:00:00 2001 From: codebien <2103732+codebien@users.noreply.github.com> Date: Mon, 16 Oct 2023 19:51:46 +0200 Subject: [PATCH 1/2] output/cloud: Set v2 as the default --- cloudapi/config.go | 16 ++++++++++++---- output/cloud/output_test.go | 4 ++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/cloudapi/config.go b/cloudapi/config.go index 823c673e67e..9b9ac19a397 100644 --- a/cloudapi/config.go +++ b/cloudapi/config.go @@ -162,7 +162,7 @@ type Config struct { // NewConfig creates a new Config instance with default values for some fields. func NewConfig() Config { - return Config{ + c := Config{ Host: null.NewString("https://ingest.k6.io", false), LogsTailURL: null.NewString("wss://cloudlogs.k6.io/api/v1/tail", false), WebAppURL: null.NewString("https://app.k6.io", false), @@ -176,14 +176,14 @@ func NewConfig() Config { MaxMetricSamplesPerPackage: null.NewInt(100000, false), Timeout: types.NewNullDuration(1*time.Minute, false), - APIVersion: null.NewInt(1, false), + APIVersion: null.NewInt(2, false), // The set value (1000) is selected for performance reasons. // Any change to this value should be first discussed with internal stakeholders. MaxTimeSeriesInBatch: null.NewInt(1000, false), - // Aggregation is disabled by default, since AggregationPeriod has no default value - // but if it's enabled manually or from the cloud service, those are the default values it will use: + // Aggregation is disabled by default for legacy version (v1), since AggregationPeriod has no default value + // but if it's enabled manually or from the cloud service, those are the default values it will use. AggregationCalcInterval: types.NewNullDuration(3*time.Second, false), AggregationWaitPeriod: types.NewNullDuration(5*time.Second, false), AggregationMinSamples: null.NewInt(25, false), @@ -196,6 +196,14 @@ func NewConfig() Config { AggregationOutlierIqrCoefLower: null.NewFloat(1.5, false), AggregationOutlierIqrCoefUpper: null.NewFloat(1.3, false), } + + // v2 enables aggregation by default + if c.APIVersion.Int64 == 2 { + c.AggregationPeriod = types.NewNullDuration(3*time.Second, false) + c.AggregationWaitPeriod = types.NewNullDuration(8*time.Second, false) + } + + return c } // Apply saves config non-zero config values from the passed config in the receiver. diff --git a/output/cloud/output_test.go b/output/cloud/output_test.go index a649ddb0b0f..05a28a76adf 100644 --- a/output/cloud/output_test.go +++ b/output/cloud/output_test.go @@ -99,7 +99,7 @@ func TestOutputCreateTestWithConfigOverwrite(t *testing.T) { "reference_id": "cloud-create-test", "config": { "metricPushInterval": "10ms", - "aggregationPeriod": "30ms" + "aggregationPeriod": "1s" } }`) case "/v1/tests/cloud-create-test": @@ -126,7 +126,7 @@ func TestOutputCreateTestWithConfigOverwrite(t *testing.T) { require.NoError(t, out.Start()) assert.Equal(t, types.NullDurationFrom(10*time.Millisecond), out.config.MetricPushInterval) - assert.Equal(t, types.NullDurationFrom(30*time.Millisecond), out.config.AggregationPeriod) + assert.Equal(t, types.NullDurationFrom(1*time.Second), out.config.AggregationPeriod) // Assert that it overwrites only the provided values expTimeout := types.NewNullDuration(60*time.Second, false) From 2339ffb19413cedb38403b857c6ba32ff12d5453 Mon Sep 17 00:00:00 2001 From: codebien <2103732+codebien@users.noreply.github.com> Date: Tue, 17 Oct 2023 12:44:57 +0200 Subject: [PATCH 2/2] Really use v2 as the default --- cloudapi/config.go | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/cloudapi/config.go b/cloudapi/config.go index 9b9ac19a397..f5ac5e2c052 100644 --- a/cloudapi/config.go +++ b/cloudapi/config.go @@ -181,26 +181,34 @@ func NewConfig() Config { // The set value (1000) is selected for performance reasons. // Any change to this value should be first discussed with internal stakeholders. MaxTimeSeriesInBatch: null.NewInt(1000, false), - - // Aggregation is disabled by default for legacy version (v1), since AggregationPeriod has no default value - // but if it's enabled manually or from the cloud service, those are the default values it will use. - AggregationCalcInterval: types.NewNullDuration(3*time.Second, false), - AggregationWaitPeriod: types.NewNullDuration(5*time.Second, false), - AggregationMinSamples: null.NewInt(25, false), - AggregationOutlierAlgoThreshold: null.NewInt(75, false), - AggregationOutlierIqrRadius: null.NewFloat(0.25, false), + // TODO: the following values were used by the previous default version (v1). + // We decided to keep the same values mostly for having a smoother migration to v2. + // Because the previous version's aggregation config, a few lines below, is overwritten + // by the remote service with the same values that we are now setting here for v2. + // When the migration will be completed we may evaluate to re-discuss them + // as we may evaluate to reduce these values - especially the waiting period. + // A more specific request about waiting period is mentioned in the link below: + // https://github.com/grafana/k6/blob/44e1e63aadb66784ff0a12b8d9821a0fdc9e7467/output/cloud/expv2/collect.go#L72-L77 + AggregationPeriod: types.NewNullDuration(3*time.Second, false), + AggregationWaitPeriod: types.NewNullDuration(8*time.Second, false), + } + + if c.APIVersion.Int64 == 1 { + // Aggregation is disabled by default for legacy version, since AggregationPeriod has no default value + // but if it's enabled manually or from the cloud service and the cloud doesn't override the config then + // those are the default values it will use. + c.AggregationPeriod = types.NewNullDuration(0, false) + c.AggregationCalcInterval = types.NewNullDuration(3*time.Second, false) + c.AggregationWaitPeriod = types.NewNullDuration(5*time.Second, false) + c.AggregationMinSamples = null.NewInt(25, false) + c.AggregationOutlierAlgoThreshold = null.NewInt(75, false) + c.AggregationOutlierIqrRadius = null.NewFloat(0.25, false) // Since we're measuring durations, the upper coefficient is slightly // lower, since outliers from that side are more interesting than ones // close to zero. - AggregationOutlierIqrCoefLower: null.NewFloat(1.5, false), - AggregationOutlierIqrCoefUpper: null.NewFloat(1.3, false), - } - - // v2 enables aggregation by default - if c.APIVersion.Int64 == 2 { - c.AggregationPeriod = types.NewNullDuration(3*time.Second, false) - c.AggregationWaitPeriod = types.NewNullDuration(8*time.Second, false) + c.AggregationOutlierIqrCoefLower = null.NewFloat(1.5, false) + c.AggregationOutlierIqrCoefUpper = null.NewFloat(1.3, false) } return c