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

Make metrics.Metric be immutable #3907

Open
mstoykov opened this issue Aug 22, 2024 · 0 comments
Open

Make metrics.Metric be immutable #3907

mstoykov opened this issue Aug 22, 2024 · 0 comments
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes refactor

Comments

@mstoykov
Copy link
Contributor

mstoykov commented Aug 22, 2024

What?

Basically the TODO here:

k6/metrics/metric.go

Lines 11 to 26 in 55eb8bc

// A Metric defines the shape of a set of data.
type Metric struct {
registry *Registry `json:"-"`
Name string `json:"name"`
Type MetricType `json:"type"`
Contains ValueType `json:"contains"`
// TODO: decouple the metrics from the sinks and thresholds... have them
// linked, but not in the same struct?
Tainted null.Bool `json:"tainted"`
Thresholds Thresholds `json:"thresholds"`
Submetrics []*Submetric `json:"submetrics"`
Sub *Submetric `json:"-"`
Sink Sink `json:"-"`
Observed bool `json:"-"`
}

The current metric has a bunch of fields used by the ingester, summary and engine to:

  1. aggregate metric samples for the summary and thresholds
  2. evelaute thresholds and it subthresholds

None of those are essentials to hte metrics and the current users of the code need to know to not touch those values.

Arguably also those fields shouldn't be exported at all

How?

Move all of those fields in the places where they are useful as well as the type definitions. Probably make them internal.

In the past users (and core developers) have used those to implement metric outputs among other things, and while it might work for some cases, it has also introduced complex bugs and have always fell short. As well that isn't the intended purpose.

Related issues/PRs:

#2735 will be part of this
#2320 will likely be closed as part of this as well

@mstoykov mstoykov added refactor breaking change for PRs that need to be mentioned in the breaking changes section of the release notes labels Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes refactor
Projects
None yet
Development

No branches or pull requests

1 participant