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

Refactor the current way metrics are defined #572

Open
na-- opened this issue Apr 5, 2018 · 3 comments
Open

Refactor the current way metrics are defined #572

na-- opened this issue Apr 5, 2018 · 3 comments
Assignees
Labels

Comments

@na--
Copy link
Member

na-- commented Apr 5, 2018

We currently define all possible emitted metrics as shared global variables. As this ugly data race fix shows, that's probably not a very good idea, since the stats.Metric struct seems to contain some mutable fields.

Before embarking on a huge refactoring spree, we should check if the go-metrics library (suggested in this issue) or another one offers us a nicer solution.

@na--
Copy link
Member Author

na-- commented Jun 9, 2022

Most of this issue has been resolved by the metrics Registry introduced by #2071 and the follow-up related PRs like #2433, #2442 and a bunch of refactoring PRs...

However, the mix of metric metadata and mutable state still remains a problem in the Metric struct, this is what the TODO here is about:

k6/metrics/metric.go

Lines 12 to 26 in 27f2ead

// A Metric defines the shape of a set of data.
type Metric struct {
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:"-"`
}

This will hopefully soon be taken care of as well. After we completely remove the core.Engine (PoC for that in #2438, 8850e35 specifically, that will soon be polished and moved into a separate PR), we'll be able to split apart that struct and move the state to only exist in the simpler metrics.MetricsEngine. We can probably also simplify the locking quite a lot then... 🤔

@na-- na-- added this to the v0.40.0 milestone Jun 9, 2022
@na-- na-- removed the bug label Jun 9, 2022
@na-- na-- modified the milestones: v0.40.0, v0.41.0 Aug 19, 2022
@na-- na-- modified the milestones: v0.41.0, TBD Oct 28, 2022
@codebien codebien self-assigned this Dec 9, 2022
@codebien
Copy link
Collaborator

codebien commented Dec 9, 2022

When we will pick this issue, we should check if it makes sense to address #2320 in the same cycle.

@codebien codebien modified the milestones: TBD, v0.45.0 Apr 26, 2023
@andrewslotin andrewslotin modified the milestones: v0.45.0, v0.46.0 May 31, 2023
@mstoykov mstoykov modified the milestones: v0.46.0, v0.47.0 Jul 31, 2023
@codebien codebien modified the milestones: v0.47.0, v0.48.0 Sep 25, 2023
@olegbespalov olegbespalov removed this from the v0.48.0 milestone Nov 16, 2023
@olegbespalov
Copy link
Collaborator

For the record. After an internal discussion, we decided to clean a milestone since the issue was jumping between milestones without completion.

Once we determine which milestone it lands, we set the right one.

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

No branches or pull requests

5 participants