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

output/cloudv2: Optimized metric sinks #3085

Merged
merged 8 commits into from
May 31, 2023
Merged

Conversation

codebien
Copy link
Contributor

It address this request change #3071 (comment)

@codebien codebien self-assigned this May 22, 2023
@github-actions github-actions bot requested review from mstoykov and oleiade May 22, 2023 12:27
This was referenced May 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2023

Codecov Report

Merging #3085 (3d15ef4) into master (7db2dbf) will increase coverage by 0.04%.
The diff coverage is 67.39%.

❗ Current head 3d15ef4 differs from pull request most recent head 986db71. Consider uploading reports for the commit 986db71 to get more accurate results

@@            Coverage Diff             @@
##           master    #3085      +/-   ##
==========================================
+ Coverage   73.69%   73.74%   +0.04%     
==========================================
  Files         241      241              
  Lines       18435    18451      +16     
==========================================
+ Hits        13585    13606      +21     
+ Misses       3977     3973       -4     
+ Partials      873      872       -1     
Flag Coverage Δ
ubuntu 73.74% <67.39%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
output/cloud/expv2/hdr.go 95.83% <0.00%> (+2.50%) ⬆️
output/cloud/expv2/mapping.go 41.79% <20.00%> (+0.94%) ⬆️
output/cloud/expv2/sink.go 92.30% <92.30%> (-7.70%) ⬇️
output/cloud/expv2/collect.go 92.15% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 🎉

I left a comment on naming and a question about trimzeros, that I don't consider blockers. I would approve as is, but I wanted to make sure those were addressed (as in "I got an answer", not as in "they made the change I wanted" 😉) first 🙇🏻

output/cloud/expv2/sink.go Outdated Show resolved Hide resolved
output/cloud/expv2/output.go Outdated Show resolved Hide resolved
output/cloud/expv2/mapping.go Outdated Show resolved Hide resolved
@codebien codebien force-pushed the ingestion/histogram branch 2 times, most recently from 5c61d6f to ede28b0 Compare May 27, 2023 13:22
@codebien codebien force-pushed the cloud-v2-optimized-sinks branch 2 times, most recently from 3d4683b to eb0c005 Compare May 27, 2023 13:44
Comment on lines +43 to +51
type gauge struct {
Last float64
Sum float64
Min, Max float64
Avg float64
Count uint32

minSet bool
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving here the discussion from #3083 (comment)

Shouldn't this be just Last? Which is what Gauge basically is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @esquonk, apparently other protocols (OTel, Prometheus remote write) don't support gauge in the same way. We may have issues if we want to migrate to them in the future. Considering this are not used at the moment, can we just drop them?

Copy link

Choose a reason for hiding this comment

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

We have those in the backend historically, and they are used. For example, if we have a "max" query for gauge metric, the code will look at "max" column.

We could just fill all the columns with "last" value at the cost losing a bit of data, but if we can collect some more statistical values, it make sense to do so. Unless it is too expensive to maintain.

As for compatibility with other protocols, I don't see how having more data internally can cause issues with that. If you need to produce an output in some protocol that only has last, you can just put last in there and ignore other values in your output.

@codebien codebien force-pushed the ingestion/histogram branch 2 times, most recently from d2add9c to 128e55a Compare May 30, 2023 08:16
Base automatically changed from ingestion/histogram to master May 30, 2023 14:24
@codebien codebien added this to the v0.45.0 milestone May 30, 2023
mstoykov
mstoykov previously approved these changes May 30, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM! in general with the understandign that Gauge is not really a Gauge because the k6 cloud backend wants it like this.

I have left some small comments, and one big one which is mostly an optimizaiton idea for after we have merged everything and this output works.

Comment on lines +9 to +11
type metricValue interface {
Add(v float64)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

for future: I kind of wonder if we should not just have a

type metricSinks struct {
  counters map[metrics.TimeSeries]counter
  gauges map[metrics.TimeSeries]gauge
  trends map[metrics.TimeSeries]histogram 
  rates map[metrics.TimeSeries]rate 
}

as a replacement for what map[metrics.TimeSeries]metricValue is basically used now.

We will drop the interface here.
We will drop a bunch of the switches including in the final write path where you will just write all the countes, all the gauges and so on. Instead of checking which is which all the time.

IMO this will be faster but we definitely should benchmark it and this code is good enough.

output/cloud/expv2/mapping.go Outdated Show resolved Hide resolved
output/cloud/expv2/mapping.go Outdated Show resolved Hide resolved
@mstoykov mstoykov requested a review from oleiade May 30, 2023 15:03
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

}

type rate struct {
NonZeroCount uint32
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: I would suggest rename this to something (I would find) more explicit: StrictlyPositiveCount instead.

Copy link
Contributor Author

@codebien codebien May 31, 2023

Choose a reason for hiding this comment

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

They are not only positive, negative are accepted, only zero is excluded. The condition:

if v != 0 {
r.NonZeroCount++
}

@codebien codebien merged commit 811d5d6 into master May 31, 2023
@codebien codebien deleted the cloud-v2-optimized-sinks branch May 31, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants