Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a dedicated option instead of MaxMetricSamplesPerPackage #3156

Closed
Tracked by #3117
codebien opened this issue Jun 29, 2023 · 1 comment · Fixed by #3157
Closed
Tracked by #3117

Use a dedicated option instead of MaxMetricSamplesPerPackage #3156

codebien opened this issue Jun 29, 2023 · 1 comment · Fixed by #3157
Assignees
Labels
Milestone

Comments

@codebien
Copy link
Contributor

codebien commented Jun 29, 2023

What

As defined by the code's comment below, we should use a better name that matches the purpose for MaxMetricSamplesPerPackage in cloudv2.

cloudv2 uses MaxMetricSamplesPerPackage for defining the max number of time series in a single flush batch. Instead, the current option defines the number of k6 samples.

k6/cloudapi/config.go

Lines 34 to 36 in 1df72a0

// TODO: rename the config field to align to the new logic by time series
// when the migration from the version 1 is completed.
MaxMetricSamplesPerPackage null.Int `json:"maxMetricSamplesPerPackage" envconfig:"K6_CLOUD_MAX_METRIC_SAMPLES_PER_PACKAGE"`

// TODO: rename the config field to align to the new logic by time series
// when the migration from the version 1 is completed.
maxSeriesInSingleBatch: int(o.config.MaxMetricSamplesPerPackage.Int64),

Why

MaxMetricSamplesPerPackage is inherited from v1 but we should use a cleaner and dedicated field.

Suggestion

Add a dedicated new field for defining the max number of series in a single flush batch.

@codebien codebien added the cloud label Jun 29, 2023
@codebien codebien added this to the v0.46.0 milestone Jun 29, 2023
@codebien codebien changed the title Replace the current usage of MaxMetricSamplesPerPackage with a dedicated and explicit variable. Beter option instead of MaxMetricSamplesPerPackage Jun 29, 2023
@codebien codebien changed the title Beter option instead of MaxMetricSamplesPerPackage Use a dedicated option instead of MaxMetricSamplesPerPackage Jun 29, 2023
@imiric
Copy link
Contributor

imiric commented Jun 29, 2023

Whatever the new name is, it shouldn't reference "packages", which I'm not sure what they are. I think this was meant to be "per (network) packet", but even that would be wrong, as HTTP is not concerned with TCP/IP packets, and requests could be split over several packets. So "per (HTTP) request" would be more accurate, or since the v2 output now deals with protobuf, maybe "per (protobuf) message", or more generically "per payload", would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants