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

Metric type refactor #2905

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Metric type refactor #2905

wants to merge 8 commits into from

Conversation

codebien
Copy link
Collaborator

@codebien codebien commented Feb 7, 2023

No description provided.

@codebien codebien self-assigned this Feb 7, 2023
@codebien codebien added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Feb 7, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2905 (c0cd986) into master (b750e20) will decrease coverage by 0.03%.
The diff coverage is 87.73%.

@@            Coverage Diff             @@
##           master    #2905      +/-   ##
==========================================
- Coverage   76.88%   76.86%   -0.03%     
==========================================
  Files         225      223       -2     
  Lines       16867    16939      +72     
==========================================
+ Hits        12969    13020      +51     
- Misses       3066     3078      +12     
- Partials      832      841       +9     
Flag Coverage Δ
ubuntu 76.86% <87.73%> (+0.04%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/v1/metric_jsonapi.go 100.00% <ø> (ø)
metrics/metric.go 92.45% <ø> (ø)
api/v1/metric.go 53.84% <65.21%> (-0.70%) ⬇️
metrics/engine/ingester.go 81.25% <83.33%> (-3.94%) ⬇️
metrics/engine/engine.go 90.44% <89.01%> (+1.56%) ⬆️
api/v1/metric_routes.go 72.72% <100.00%> (-5.06%) ⬇️
cmd/run.go 71.20% <100.00%> (ø)
js/summary.go 90.24% <100.00%> (+0.37%) ⬆️
metrics/registry.go 95.12% <100.00%> (+3.63%) ⬆️
metrics/sink.go 94.91% <100.00%> (+0.17%) ⬆️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

type ObservedMetric struct {
*Metric
Sink Sink
Thresholds []Threshold
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to tie thresholds to a particular metric? 🤔 This will prevent us from having thresholds that reference multiple metrics in the future (#1441), and maybe even from improvements like #1253

"max": sink.Max,
"avg": sink.Avg,
"med": sink.P(0.5),
"p(90)": sink.P(0.90),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good opportunity to fix #1253?

// sequential integer ID, we would be able to use a slice for these buckets
// and eliminate the map loopkups altogether!

samplesByMetric := make(map[*metrics.Metric][]metrics.Sample)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inefficient, maybe we can avoid using output.SampleBuffer and do this in a custom AddMetricSamples()

Comment on lines 57 to 58
metricsWithThresholds map[string]metrics.Thresholds
trackedMetrics map[string]*trackedMetric
Copy link
Collaborator Author

@codebien codebien Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to investigate as a better and long-term solution the metric.SequenceID option:

  • We need to check if we can continue to keep in the long-term the sub-metrics as metrics.

@codebien codebien force-pushed the refactor/tracked-metrics branch 2 times, most recently from 908a243 to b25d18d Compare February 10, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants