build generic time series #16402
build generic time series #16402
Conversation
Signed-off-by: perebaj <perebaj@gmail.com>
aeeecb2 to
8cd3b82
Compare
Signed-off-by: perebaj <perebaj@gmail.com>
Signed-off-by: perebaj <perebaj@gmail.com>
|
|
||
| keepIdx := 0 | ||
| lowest = math.MaxInt64 | ||
| func buildWriteRequest(logger *slog.Logger, timeSeries []prompb.TimeSeries, metadata []prompb.MetricMetadata, pBuf *proto.Buffer, filter func(prompb.TimeSeries) bool, buf compression.EncodeBuffer, compr compression.Type) (_ []byte, highest, lowest int64, _ error) { |
There was a problem hiding this comment.
While we refactor, is there anything we can do to reduce number of arguments. Maybe have a requestBuilder object?
storage/remote/queue_manager.go
Outdated
| highest, lowest, timeSeries, | ||
| droppedSamples, droppedExemplars, droppedHistograms := buildTimeSeries(timeSeries, filter) | ||
|
|
||
| highest, lowest, wrappedTimeSeries, droppedSamples, droppedExemplars, droppedHistograms := buildGenericTimeSeries(wrappedTimeSeries, wrappedFilter) |
There was a problem hiding this comment.
Same with return arguments, this is generally not ideal (: It's ok to say "no, let's do this in the next iteration"
There was a problem hiding this comment.
Do you mind if we take care of it in the next PR? I'm not sure if the approach that I'm taught is the best one. I would like to discuss/receive more comments about the generalization before diving into this "minors."
WDYT?
Some for this comment here
storage/remote/queue_manager.go
Outdated
| if droppedSamples > 0 || droppedExemplars > 0 || droppedHistograms > 0 { | ||
| logger.Debug("dropped data due to their age", "droppedSamples", droppedSamples, "droppedExemplars", droppedExemplars, "droppedHistograms", droppedHistograms) | ||
| } | ||
|
|
||
| // Convert back to prompb.TimeSeries |
There was a problem hiding this comment.
Comments ideally are full sentences, so they end with a period.
| // Convert back to prompb.TimeSeries | |
| // Convert back to prompb.TimeSeries. |
There was a problem hiding this comment.
Already take care of it here -> aa0c73a
Signed-off-by: perebaj <perebaj@gmail.com>
Signed-off-by: perebaj <perebaj@gmail.com>
|
Just sharing some thoughts My plan to DRY the whole code here is to submit some small chunks of changes. This PR creates a generic interface that deals with samples, exemplars, and histograms for both To DRY the whole queue_manager.go, we need to migrate the below "duplicated" functions. They are ordered according to the dependency graph and should respect this order to minimize disruption.
Maybe would be nice to add this checklist on the original issue. @bwplotka Is that make sense? |
Signed-off-by: perebaj <perebaj@gmail.com>
| type TimeSeriesData interface { | ||
| // getSamples returns the samples in the time series | ||
| getSamples() []any | ||
| // getExemplars returns the exemplars in the time series | ||
| getExemplars() []any | ||
| // getHistograms returns the histograms in the time series | ||
| getHistograms() []any | ||
| } |
There was a problem hiding this comment.
My main concern with this approach is whether returning a []any is the best solution. Since this forces every later interaction with a time series to explicitly check and convert the type, it increases the risk of runtime errors and reduces type safety. This pattern can also make the code harder to maintain because each consumer must validate and cast to the correct underlying type in order to access specific fields or methods.
I’d like feedback on better ways to achieve flexibility here 😀
I know that the only way to achieve this is using more generics. But I'm afraid to create a Frankenstein. Am I overengineering or/and overthinking, and the current approach is fair enough?
Which issue(s) does the PR fix: Partially fixes #14409
This PR DRY the queue_manager.go code, creating a generic function to build timeseries (prompb.TimeSeries or writev2.TimeSeries)
I found this issue on the presentation shared by @bwplotka
Does this PR introduce a user-facing change?