[PRW 2.0] (part3) Moved type specific conversions to prompb and writev2 codecs.#14347
[PRW 2.0] (part3) Moved type specific conversions to prompb and writev2 codecs.#14347bwplotka merged 1 commit intoremote-write-2.0from
Conversation
bcc3595 to
97ffc66
Compare
f8f8535 to
c5faeb9
Compare
523097b to
b244424
Compare
|
|
||
| // LabelProtosToLabels transforms prompb labels into labels. The labels builder | ||
| // will be used to build the returned labels. | ||
| func LabelProtosToLabels(b *labels.ScratchBuilder, labelPairs []prompb.Label) labels.Labels { |
There was a problem hiding this comment.
This affects #14316 cc @pracucci in some way, by removing this helper from this file.
We moved those methods, and they are accessible via prompb.FromLabels and prompb.TimeSeries.ToLabels. I think it makes things much cleaner and should work for downstream projects. Do you mind double checking if you are happy with this Marco? 🤗 (this PR is against remote-write-2.0 but we will merge this branch to main next week likely)
There was a problem hiding this comment.
Thanks for the ping Bartek. I'm good with this change, thanks.
| // NOTE(bwplotka): This file's code is tested in /prompb/rwcommon. | ||
|
|
||
| // ToLabels return model labels.Labels from timeseries' remote labels. | ||
| func (m TimeSeries) ToLabels(b *labels.ScratchBuilder, _ []string) labels.Labels { |
There was a problem hiding this comment.
FYI @beorn7 @krajorama we cleaned up histogram conversions as it's gets tricky (duplicated) with another proto struct. Would you be happy with flow like this (see PR description).
There was a problem hiding this comment.
I guess @krajorama is way deeper into practical considerations here than I, so he should make the call.
|
/prombench main |
krajorama
left a comment
There was a problem hiding this comment.
Makes sense, I like how the naming is cleaner. We should probably follow the same in Mimir.
I've added some nits. Also there's already some generator functions in tsdb/tsdbutil/histogram.go for generating test histograms, but using your own is fine as well
storage/remote/codec_test.go
Outdated
| req.Timeseries[i].Histograms[j].PositiveDeltas = h.PositiveDeltas | ||
| req.Timeseries[i].Histograms[j].PositiveCounts = h.PositiveCounts | ||
| req.Timeseries[i].Histograms[j].ResetHint = prompb.Histogram_ResetHint(h.ResetHint) | ||
| req.Timeseries[i].Histograms[j].Timestamp = h.Timestamp |
There was a problem hiding this comment.
Let's add a panic() if this is called on a histogram with custome_values filled in. Just to be on the safe side.
There was a problem hiding this comment.
Good point. Let's add TODOs to conversion code. For this conversion (I simplified it here), it;s only used in tests so we know it's not custom. Let's fix NHCB cases once on main branch?
|
No big diffs in Mem, CPU and latencies:
|
|
/prombench cancel |
|
Benchmark cancel is in progress. |
b244424 to
8a6ce53
Compare
…v2 codecs. Signed-off-by: bwplotka <bwplotka@gmail.com>
8a6ce53 to
0dd176b
Compare
This attempts to improve readability and consistency of our rw code & tests. For example to make sure conversions from/to model types are well tested.
We can't use generics easily due to details mentioned in Slack and old attempt so I propose we take it slowly and for now clean conversion code by:
prompb/rwcommonfor shared proto logic. For now I propose we test the above conversions in one place. The reason is that while having impl hidden in each version is beneficial, hiding tests is bit worse - it can be non-obvious when tests drifts for those. Thisrwcommonpkg can be also later used for more stuff e.g. shared types or interfaces/generics.NOTE: For fun I shared one alternative here with generics. I think it's ... bit too much for 2 types, but maybe those generic interfaces would help further on with buildWriteRequest stuff. For just testing, it's too much and complex.