output/cloudv2: Trend as Histogram #3027

codebien · 2023-04-20T16:37:32Z

Custom Histogram representation of the Trend metric type. It is the porting of the Histogram generation on the client side.

codecov-commenter · 2023-04-21T16:25:46Z

Codecov Report

Merging #3027 (fbf36a1) into master (31d9302) will increase coverage by 0.10%.
The diff coverage is 91.02%.

❗ Current head fbf36a1 differs from pull request most recent head f764955. Consider uploading reports for the commit f764955 to get more accurate results

@@            Coverage Diff             @@
##           master    #3027      +/-   ##
==========================================
+ Coverage   73.57%   73.68%   +0.10%     
==========================================
  Files         240      241       +1     
  Lines       18364    18436      +72     
==========================================
+ Hits        13512    13585      +73     
+ Misses       3980     3978       -2     
- Partials      872      873       +1

Flag	Coverage Δ
ubuntu	`73.68% <91.02%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
output/cloud/expv2/mapping.go	`40.84% <0.00%> (+0.56%)`	⬆️
output/cloud/expv2/sink.go	`100.00% <ø> (+50.00%)`	⬆️
output/cloud/expv2/hdr.go	`93.42% <93.42%> (ø)`

... and 1 file with indirect coverage changes

output/cloud/expv2/hdr.go

output/cloud/expv2/output.go

output/cloud/expv2/hdr.go

na-- · 2023-05-18T14:01:29Z

output/cloud/expv2/hdr.go

+	// let n = msb(u) - most significant digit position
+	// i.e. n = floor(log(u, 2))
+	//   major_bucket_index = n - k + 1
+	//   sub_bucket_index = u>>(n - k) - (1<<k)


Adding this mostly as a reminder to myself to double-check next week, since I don't immediately understand this line - why can sub_bucket_index be calculated like this? 🤔

The nth bucket contains [2^(n-1) : 2^n] values, equally divided in 2^k sub-buckets, right? So, I'd expect something like "zero out up to the nth bit to get the part of the number that needs to be divided into sub-buckets, divide it by 2^k to get the sub-bucket index".

But I'm probably missing something stupid, so if you have an explanation handy, please add it to the comment

mstoykov · 2023-05-29T14:10:37Z

output/cloud/expv2/hdr.go

+const (
+	// lowestTrackable represents the minimum value that the histogram tracks.
+	// Essentially, it excludes negative numbers.
+	// Most of metrics tracked by histograms are durations
+	// where we don't expect negative numbers.
+	//
+	// In the future, we may expand and include them,
+	// probably after https://github.com/grafana/k6/issues/763.
+	lowestTrackable = 0
+
+	// highestTrackable represents the maximum
+	// value that the histogram is able to track with high accuracy (0.1% of error).
+	// It should be a high enough
+	// and rationale value for the k6 context; 2^30 = 1_073_741_824
+	highestTrackable = 1 << 30
+)


This IMO should be very well communicated and documented somewhere as I am not certain these were restrictions so far.

I doubt that many users use that big values or negative ones.

Is there a reason why we are removing negative values to begin with? As in this algorithm just does not work with them?

I would also link the algorithm used in:

the code

the commit

the PR

(likely) the docs when they are added

Is there a reason why we are removing negative values to begin with? As in this algorithm just does not work with them?

My understanding of the original design is we did it with the assumption that most of the time we measure duration, and they are not negative. So it simplified the requirement for the algorithm.

output/cloud/expv2/hdr.go

mstoykov · 2023-05-29T14:51:18Z

output/cloud/expv2/hdr.go

+		if index < h.FirstNotZeroBucket {
+			h.growLeft(index)
+			h.FirstNotZeroBucket = index
+		}
+		if index > h.LastNotZeroBucket {
+			h.growRight(index)
+			h.LastNotZeroBucket = index
+		}


Very likely for a future optimization:

For every given timeseries this bucket will likely just be recreated each bucket. This can potentially mean many allocations of slices over and over again across all time series for the whole duration of the test.

There are quite a few possibilities to mitigate this:

obviously do nothing and see how it goes :)

reuse the same histogram and all of its' buckets. This likely will be easier if histogramAsProto is called when the metrics are flushed and the whole instance is just set for the next bucket after it ... this seems very iffy to me.

On each creation of a new histogram we can check if the previous bucket has a histogram for the same timeseries - copy its buckets (without the values obviously). This likely will be easier and we only pay the costs when a new histogram is added.

something else that I forgot while typing :(

Something else we come up with

I will expect this is a common problem so maybe someone has already solved it in the golang space.

The hdrhistogram-go repo has Snapshot (and a much bigger Histogram) which seems like a way to get values (snapshot) for some time and then continue to aggregate without needing to create the buckets from scratch.

Although in their case it is just a continues aggregation while we want to reset.

output/cloud/expv2/hdr.go

Not allocate anymore buckets before and after the first and the last non-zero bucket. It saves a bunch of memory.

output/cloud/expv2/hdr.go

Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>

output/cloud/expv2/hdr.go

mstoykov

LGTM!

I have left a naming nitpick which we can address later or never for that matter 😅

output/cloud/expv2/hdr.go

codebien added this to the v0.45.0 milestone Apr 20, 2023

codebien requested a review from artych April 20, 2023 16:37

codebien self-assigned this Apr 20, 2023

codebien force-pushed the ingestion/binary-proto branch from fdc5307 to bfff8af Compare April 21, 2023 15:59

codebien force-pushed the ingestion/histogram branch from 78c9580 to 1a45956 Compare April 21, 2023 16:23

codebien force-pushed the ingestion/binary-proto branch 2 times, most recently from 8396f62 to d7df637 Compare April 21, 2023 16:43

codebien force-pushed the ingestion/histogram branch 2 times, most recently from 63df8df to fb719c9 Compare April 24, 2023 15:01

codebien force-pushed the ingestion/binary-proto branch from d7df637 to 34d36f7 Compare April 24, 2023 15:02

codebien changed the title ~~output/cloud: Trend as Histogram~~ output/cloudv2: Trend as Histogram Apr 24, 2023

codebien marked this pull request as ready for review April 24, 2023 15:03

codebien requested a review from na-- April 24, 2023 15:04

codebien force-pushed the ingestion/binary-proto branch from 34d36f7 to 580f02b Compare April 27, 2023 16:56

na-- reviewed May 3, 2023

View reviewed changes

output/cloud/expv2/hdr.go Outdated Show resolved Hide resolved

na-- reviewed May 3, 2023

View reviewed changes

output/cloud/expv2/hdr.go Outdated Show resolved Hide resolved

na-- reviewed May 3, 2023

View reviewed changes

output/cloud/expv2/output.go Outdated Show resolved Hide resolved

codebien force-pushed the ingestion/binary-proto branch from 580f02b to 4c09c5e Compare May 12, 2023 14:37

This was referenced May 15, 2023

output/cloudv2: Aggregation #3071

Merged

cloud: New output v2 #3072

Merged

codebien force-pushed the ingestion/histogram branch from fb719c9 to 40ad58a Compare May 16, 2023 22:28

codebien requested review from na-- and oleiade May 16, 2023 22:31

codebien force-pushed the ingestion/binary-proto branch from 4c09c5e to 2fb9e2d Compare May 18, 2023 09:09

codebien force-pushed the ingestion/histogram branch from 40ad58a to 6e4d987 Compare May 18, 2023 12:04

na-- reviewed May 18, 2023

View reviewed changes

output/cloud/expv2/hdr.go Outdated Show resolved Hide resolved

na-- reviewed May 18, 2023

View reviewed changes

codebien force-pushed the ingestion/binary-proto branch 2 times, most recently from b3821f5 to 46f3fdf Compare May 21, 2023 18:02

codebien force-pushed the cloud-v2-flushing branch from e2152b8 to 1d7eed8 Compare May 29, 2023 11:13

codebien force-pushed the ingestion/histogram branch from ede28b0 to d2add9c Compare May 29, 2023 13:50

mstoykov reviewed May 29, 2023

View reviewed changes

Base automatically changed from cloud-v2-flushing to master May 30, 2023 08:11

codebien dismissed oleiade’s stale review via d2add9c May 30, 2023 08:11

codebien added 5 commits May 30, 2023 10:16

cloudv2: Trend conversion as HDR Histogram

f47acbf

Call Protobuf conversion from the flush process

ab5fac6

Cover from the integration test

c733058

Store only significant buckets

05ceb0d

Not allocate anymore buckets before and after the first and the last non-zero bucket. It saves a bunch of memory.

Improve the documentation

128e55a

codebien force-pushed the ingestion/histogram branch from d2add9c to 128e55a Compare May 30, 2023 08:16

codebien added 2 commits May 30, 2023 13:26

Address review comments

a45410e

Init on the constructor

e01da6c

codebien requested a review from mstoykov May 30, 2023 11:50

mstoykov reviewed May 30, 2023

View reviewed changes

output/cloud/expv2/hdr.go Outdated Show resolved Hide resolved

Update output/cloud/expv2/hdr.go

6f7acd9

Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>

mstoykov reviewed May 30, 2023

View reviewed changes

output/cloud/expv2/hdr.go Outdated Show resolved Hide resolved

mstoykov requested a review from oleiade May 30, 2023 13:29

Set index limits in the grow funcs

f63d2e6

codebien force-pushed the ingestion/histogram branch from 4846b3c to f63d2e6 Compare May 30, 2023 13:34

mstoykov previously approved these changes May 30, 2023

View reviewed changes

output/cloud/expv2/hdr.go Show resolved Hide resolved

move first addition in an init function

f764955

codebien dismissed mstoykov’s stale review via f764955 May 30, 2023 13:49

mstoykov previously approved these changes May 30, 2023

View reviewed changes

Rename buckets operations

a02fa67

codebien dismissed mstoykov’s stale review via a02fa67 May 30, 2023 14:04

mstoykov approved these changes May 30, 2023

View reviewed changes

oleiade approved these changes May 30, 2023

View reviewed changes

codebien merged commit 7db2dbf into master May 30, 2023

codebien deleted the ingestion/histogram branch May 30, 2023 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

output/cloudv2: Trend as Histogram #3027

output/cloudv2: Trend as Histogram #3027

codebien commented Apr 20, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

na-- May 18, 2023 •

edited

Loading

mstoykov May 29, 2023

mstoykov May 29, 2023

codebien May 29, 2023

mstoykov May 29, 2023

mstoykov left a comment

output/cloudv2: Trend as Histogram #3027

output/cloudv2: Trend as Histogram #3027

Conversation

codebien commented Apr 20, 2023 • edited Loading

codecov-commenter commented Apr 21, 2023 • edited Loading

Codecov Report

na-- May 18, 2023 • edited Loading

Choose a reason for hiding this comment

mstoykov May 29, 2023

Choose a reason for hiding this comment

mstoykov May 29, 2023

Choose a reason for hiding this comment

codebien May 29, 2023

Choose a reason for hiding this comment

mstoykov May 29, 2023

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Apr 20, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

na-- May 18, 2023 •

edited

Loading