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

WIP: keep track of samples per query, set a max # of samples #4513

Merged
merged 7 commits into from Oct 2, 2018

Conversation

Projects
None yet
3 participants
@cstyan
Copy link
Contributor

cstyan commented Aug 16, 2018

For #4414

Mostly looking for feedback on whether or not I'm on the right track here, naive solution for now.
@brian-brazil @tomwilkie

Signed-off-by: Callum Stytan callumstyan@gmail.com

sample.Point.T = ts
ss.Points = append(ss.Points, sample.Point)
seriess[h] = ss
if curSamples < maxSamples {

This comment has been minimized.

@cstyan

cstyan Aug 16, 2018

Contributor

it seems in some cases that there's the potential for no samples to be added in the above block of for loops, and so they're then added later down here, so we have to track curSamples here as well

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

This is the output part of the code, the above clock is input. You also need to catch the instant vector case

This comment has been minimized.

@cstyan

cstyan Aug 16, 2018

Contributor

Okay, so the output is the correct place to do it then? Or should we limit the input as well? The goal is to reduce the chance of OOM by limit the # of samples returned by a query, in this case are we not doubling up on memory allocation with the vectors for the input to the functions f, and then building the output?

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

You could view it a few ways. It probably doesn't hurt to limit both.

This comment has been minimized.

@cstyan

cstyan Aug 16, 2018

Contributor

So in some cases, the input (or rather, output of f) is the only input that we consider when building the output vector. We don't want every function passed to rangeEval to have to deal with limiting the # of samples right?

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

Yes, only matrix functions and raw lookups should need special handling beyond that

@brian-brazil
Copy link
Member

brian-brazil left a comment

Just a quick look

@@ -382,7 +384,7 @@ func (ng *Engine) execEvalStmt(ctx context.Context, query *query, s *EvalStmt) (
ctx: ctx,
logger: ng.logger,
}
val, err := evaluator.Eval(s.Expr)
val, err := evaluator.Eval(s.Expr, ng.maxSamplesPerQuery)

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

Why pass this down here rather than when creating the evaluator? Passing it everywhere is a bit ugly

This comment has been minimized.

@cstyan

cstyan Aug 16, 2018

Contributor

Agreed, it's already on my "todo list". I was mostly just exploring how the code was currently working to start.

}

// NewEngine returns a new engine.
func NewEngine(logger log.Logger, reg prometheus.Registerer, maxConcurrent int, timeout time.Duration) *Engine {
func NewEngine(logger log.Logger, reg prometheus.Registerer, maxConcurrent int, maxSamples int, timeout time.Duration) *Engine {

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

It might be time to make this a struct

This comment has been minimized.

@tomwilkie

tomwilkie Aug 17, 2018

Member

Yes please!

@@ -1116,14 +1149,20 @@ func (ev *evaluator) matrixIterSlice(it *storage.BufferedSeriesIterator, mint, m
}
// Values in the buffer are guaranteed to be smaller than maxt.
if t >= mint {
out = append(out, Point{T: t, V: v})
if curSamples < maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

You should be failing if the limit is exceeded, not silently dropping data

This comment has been minimized.

@cstyan

cstyan Aug 16, 2018

Contributor

Failing the query entirely, and returning nothing?

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

Yes, same as a timeout.

This comment has been minimized.

@cstyan

cstyan Aug 16, 2018

Contributor

interesting

sample.Point.T = ts
ss.Points = append(ss.Points, sample.Point)
seriess[h] = ss
if curSamples < maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Aug 16, 2018

Member

This is the output part of the code, the above clock is input. You also need to catch the instant vector case

@@ -574,11 +578,11 @@ type evaluator struct {
ctx context.Context

startTimestamp int64 // Start time in milliseconds.
endTimestamp int64 // End time in milliseconds.
interval int64 // Interval in milliseconds.

This comment has been minimized.

@tomwilkie

tomwilkie Aug 17, 2018

Member

Try to avoid unnecessary changes like this (deleted a line), it makes the diff harder to read.

This comment has been minimized.

@cstyan

cstyan Aug 17, 2018

Contributor

yeah, wish the diff output was smarter here 👍

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from 6826360 to e40e10a Aug 20, 2018

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Aug 22, 2018

@brian-brazil @tomwilkie PTAL, rebased off master since there were some merge conflicts in promql/engine

@@ -186,6 +187,8 @@ func main() {

a.Flag("query.max-concurrency", "Maximum number of queries executed concurrently.").
Default("20").IntVar(&cfg.queryConcurrency)
a.Flag("query.max-samples", "Maximum number of samples a single query can return. Queries will fail if they would return more than this number.").

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

This message isn't right, it's about samples in memory rather than returned.

Could we express it in bytes? It's a lower bound due to things like labelsets and capacity vs len, but we could improve it over time.

@@ -186,6 +187,8 @@ func main() {

a.Flag("query.max-concurrency", "Maximum number of queries executed concurrently.").
Default("20").IntVar(&cfg.queryConcurrency)
a.Flag("query.max-samples", "Maximum number of samples a single query can return. Queries will fail if they would return more than this number.").
Default("1000000").IntVar(&cfg.queryMaxSamples)

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

This should be at least 20M

@@ -67,13 +68,22 @@ type (
ErrQueryTimeout string
// ErrQueryCanceled is returned if a query was canceled during processing.
ErrQueryCanceled string
// ErrTooManySamples is returned if a query would return more than the maximum allowed samples.

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

Samples in memory, not returned

numSteps := int((ev.endTimestamp-ev.startTimestamp)/ev.interval) + 1
matrixes := make([]Matrix, len(exprs))
origMatrixes := make([]Matrix, len(exprs))
// Limit both input and output samples, in some cases we allocate input vectors.
// We'll want to know that the input is too large and fail early rather than also

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

Hmm, shouldn't we already have failed while preparing the inputs?

This comment has been minimized.

@cstyan

cstyan Aug 23, 2018

Contributor

hmm yeah, I though't I'd already removed this comment when I removed the outputSamples tracking

vectors[i] = append(vectors[i], Sample{Metric: series.Metric, Point: point})
// Move input vectors forward so we don't have to re-scan the same
// past points at the next step.
matrixes[i][si].Points = series.Points[1:]
inputSamples++

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

That's not right, the input samples are the entire of the input matrix - not jut the bits we've used so far.

This comment has been minimized.

@cstyan

cstyan Aug 23, 2018

Contributor

I'm not sure what you mean, because we already have matrixes in memory? We're only appending the samples we end up using to vectors, which is then used as args to f,

This comment has been minimized.

@brian-brazil

brian-brazil Aug 24, 2018

Member

Yes, they're already in memory.

This comment has been minimized.

@cstyan

cstyan Aug 24, 2018

Contributor

So then if the length of all the Matrix in matrixes is > maxSamples should we not just fail right away? Or only if inputSamples + len(matrixes[i]) > maxSamples?

This comment has been minimized.

@brian-brazil

brian-brazil Aug 24, 2018

Member

We should have already failed when one of them was being calculated.

This comment has been minimized.

@cstyan

cstyan Aug 24, 2018

Contributor

Right, so if inputSamples + len matrixes[i] > maxSamples we should fail, that should happen on line 707

This comment has been minimized.

@brian-brazil

brian-brazil Aug 24, 2018

Member

Yeah around, there. It'd be inside the eval.

This comment has been minimized.

@cstyan

cstyan Aug 24, 2018

Contributor

Well we're already tracking if each call to eval would load more than maxSamples, but not if the result of all the calls in rangeEval to eval would.

This comment has been minimized.

@cstyan

cstyan Aug 24, 2018

Contributor

So it seems if we track the # of samples that way, based on the outputs of the eval's, we don't need to track samples where I am here at all. That, in combination with not tracking samples when adding to the output series (29e1469), are we missing any cases? The engine tests I added would all still pass.

This comment has been minimized.

@brian-brazil

brian-brazil Aug 27, 2018

Member

I'm not seeing where you're taking into account the size of the input matrixes here, them plus this, plus the output could be too big.

otherInArgs[i] = Vector{Sample{}}
inArgs[i] = otherInArgs[i]
}
}

sel := e.Args[matrixArgIndex].(*MatrixSelector)
mat := make(Matrix, 0, len(sel.series)) // Output matrix.
mat := make(Matrix, 0, ev.maxSamples) // Output matrix.

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

This would make the matrix way too large, the existing capacity was fine.

@@ -1112,14 +1190,20 @@ func (ev *evaluator) matrixIterSlice(it *storage.BufferedSeriesIterator, mint, m
}
// Values in the buffer are guaranteed to be smaller than maxt.
if t >= mint {
out = append(out, Point{T: t, V: v})
if curSamples < ev.maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Aug 23, 2018

Member

This should be erroring

This comment has been minimized.

@cstyan

cstyan Aug 24, 2018

Contributor

fixed

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from e40e10a to 8d43a66 Aug 23, 2018

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from 720d97a to 515793e Aug 24, 2018

@brian-brazil
Copy link
Member

brian-brazil left a comment

This is still not covering the major cases, I'd suggest taking a peak at the other PR

vectors[i] = append(vectors[i], Sample{Metric: series.Metric, Point: point})
// Move input vectors forward so we don't have to re-scan the same
// past points at the next step.
matrixes[i][si].Points = series.Points[1:]
inputSamples++

This comment has been minimized.

@brian-brazil

brian-brazil Aug 27, 2018

Member

I'm not seeing where you're taking into account the size of the input matrixes here, them plus this, plus the output could be too big.

@@ -720,14 +747,19 @@ func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ..
enh.ts = ts
result := f(args, enh)
enh.out = result[:0] // Reuse result vector.

if len(result) > ev.maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Aug 27, 2018

Member

It's not just the result - it's the inputs to it too

This comment has been minimized.

@cstyan

cstyan Aug 27, 2018

Contributor

isn't the function f further limiting the # of samples we have? but I guess we already have the input vectors, and then we have the result of f as well, both in memory?

This comment has been minimized.

@brian-brazil

brian-brazil Aug 28, 2018

Member

Yes, both need to be considered.

This comment has been minimized.

@brian-brazil

brian-brazil Aug 28, 2018

Member

Yes, both are present. If there's binary operators, there may also be input vectors from the LHS of binary operators up the call stack.

otherInArgs[i] = Vector{Sample{}}
inArgs[i] = otherInArgs[i]
}
}

sel := e.Args[matrixArgIndex].(*MatrixSelector)
mat := make(Matrix, 0, len(sel.series)) // Output matrix.
curSamples := 0

This comment has been minimized.

@brian-brazil

brian-brazil Aug 27, 2018

Member

Input vectors should be considered too

This comment has been minimized.

This comment has been minimized.

@brian-brazil
// function call results.
func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ...Expr) Matrix {
func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ...Expr) (Matrix, error) {

This comment has been minimized.

@brian-brazil

brian-brazil Aug 27, 2018

Member

You shouldn't need to change this signature, use ev.error

This comment has been minimized.

@cstyan

cstyan Aug 27, 2018

Contributor

we want to panic and recover all error cases? that seems strange to me

This comment has been minimized.

@brian-brazil

brian-brazil Aug 27, 2018

Member

Yes, that's how error handling is done in PromQL.

@cstyan cstyan force-pushed the cstyan:callum-4414 branch 2 times, most recently from a3b030a to d9e9a77 Aug 27, 2018

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Aug 29, 2018

sorry for the delay, I think I'm covering all the cases now but I'll have another look over my PR plus the other PR, I also want to simplify the sample tracking by keeping track of it in the evaluator struct rather than via an int in each function

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Aug 30, 2018

@brian-brazil when you have time please see the latest commit. I've simplified the tracking of the # samples. The # of samples in memory vs the # of samples being returned from an evaluation was tripping me up a bit still, but I think I've got things right this time. I've left comments where I was previously tracking samples but no longer think it's necessary, or where we're not tracking samples in the first place because I think it's not necessary.

I think the metrics around query sizes in the other PR were interesting so I'll add some here.

If everything's good I'll rebase down to one commit.

@brian-brazil
Copy link
Member

brian-brazil left a comment

That looks more like it, however you never decrease currentSamples. For example the inputs to a function are no longer in memory once the function's rangeEval is complete.

return fmt.Sprintf("query was canceled in %s", string(e))
}
func (e ErrTooManySamples) Error() string {
return fmt.Sprintf("query would return too many samples in %s", string(e))

This comment has been minimized.

@brian-brazil

brian-brazil Sep 5, 2018

Member

it's more the processing than the end result

@@ -677,7 +700,8 @@ func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ..
for i, e := range exprs {
// Functions will take string arguments from the expressions, not the values.
if e != nil && e.Type() != ValueTypeString {
matrixes[i] = ev.eval(e).(Matrix)
ret := ev.eval(e)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 5, 2018

Member

You don't need to change this

This comment has been minimized.

@cstyan

cstyan Sep 5, 2018

Contributor

👍 sorry, leftover from when I modified the function signatures to return errors

Show resolved Hide resolved promql/engine.go Outdated
Show resolved Hide resolved promql/engine.go Outdated
// in the above for loop that built the input vectors.
ev.currentSamples += len(result)
if ev.currentSamples > ev.maxSamples {
ev.error(error(ErrTooManySamples(env)))

This comment has been minimized.

@brian-brazil

brian-brazil Sep 5, 2018

Member

Why do you need the error() here?

This comment has been minimized.

@cstyan

cstyan Sep 5, 2018

Contributor

do we not have more samples in memory via the return value of f?

This comment has been minimized.

@brian-brazil

brian-brazil Sep 6, 2018

Member

I mean specifically, why are you wrapping ErrTooManySamples in error?

This comment has been minimized.

@cstyan

cstyan Sep 6, 2018

Contributor

Hmm, at some point I had a compile error with this but you're right they're not needed.

@@ -777,6 +780,7 @@ func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ..
seriess[h] = ss

}
ev.currentSamples = tempNumSamples

This comment has been minimized.

@brian-brazil

brian-brazil Sep 7, 2018

Member

This can return early up a bit, so this needs reworking.

The input vectors to the function also need accounting for when they're no longer in use.

This comment has been minimized.

@cstyan

cstyan Sep 7, 2018

Contributor

The input vectors to the function also need accounting for when they're no longer in use.

Not sure what you mean here. We should reset the number of samples immediately after the call to f rather than later on?

This comment has been minimized.

@brian-brazil

brian-brazil Sep 8, 2018

Member

With recursive calls to eval, once the inner evals are done we don't need to account for their memory.

matrixes[i] = ev.eval(e).(Matrix)
// Once inner calls to eval are done we don't need to account for memory for

This comment has been minimized.

@brian-brazil

brian-brazil Sep 11, 2018

Member

The output from those calls does still count though.

This comment has been minimized.

@cstyan

cstyan Sep 11, 2018

Contributor

Right, the output is saved in matrixes[i] and then we're tracking the number of samples in matrixes below in the loop for the timestamp. We should be adding len(matrixes) to ev.currentSamples after resetting to tempNumSamples I think.

This comment has been minimized.

@cstyan

cstyan Sep 11, 2018

Contributor

I think this is right.

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from 830081b to 9aa7c9a Sep 11, 2018

@@ -186,6 +187,8 @@ func main() {

a.Flag("query.max-concurrency", "Maximum number of queries executed concurrently.").
Default("20").IntVar(&cfg.queryConcurrency)
a.Flag("query.max-samples", "Maximum number of samples a single query can load into memory. Note that queries will fail if they would load more samples than this into memory, so this also limits the number of samples a query can return.").
Default("1000000").IntVar(&cfg.queryMaxSamples)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 12, 2018

Member

I'd suggest 50M, that's nominally 800MB

@@ -889,12 +944,14 @@ func (ev *evaluator) eval(expr Expr) Value {
return ev.eval(e.Expr)

case *UnaryExpr:
mat := ev.eval(e.Expr).(Matrix)
ret = ev.eval(e.Expr)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 12, 2018

Member

This can be undone

if len(ss.Points) > 0 {
mat = append(mat, ss)
// (callum) I think we still need to track number of samples here
if len(ss.Points) > 0 && ev.currentSamples < ev.maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Sep 12, 2018

Member

This seems more complicated than it needs to be

@@ -818,8 +865,10 @@ func (ev *evaluator) eval(expr Expr) Value {
otherInArgs := make([]Vector, len(e.Args))
for i, e := range e.Args {
if i != matrixArgIndex {
otherArgs[i] = ev.eval(e).(Matrix)
ret = ev.eval(e)
otherArgs[i] = ret.(Matrix)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 12, 2018

Member

This can be undone

// Once inner calls to eval are done we don't need to account for memory for
// samples within those calls. However the result of the call to eval is in mem.
ev.currentSamples = tempNumSamples
ev.currentSamples += len(matrixes[i])

This comment has been minimized.

@brian-brazil

brian-brazil Sep 12, 2018

Member

This is the number of series, not the number of samples. I think you're doing the saving of tempNumSamples in the wrong place, and you don't need any code changes to this part as ev.currentSamples will already have the right value.

Show resolved Hide resolved promql/engine.go Outdated
// function call results.
func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ...Expr) Matrix {
numSteps := int((ev.endTimestamp-ev.startTimestamp)/ev.interval) + 1
matrixes := make([]Matrix, len(exprs))
origMatrixes := make([]Matrix, len(exprs))
tempNumSamples := 0

This comment has been minimized.

@brian-brazil

brian-brazil Sep 12, 2018

Member

Shouldn't this be ev.currentSamples? Ultimately when this function returns ev.currentSamples should be the previous value, plus how ever many samples we're returning.

This comment has been minimized.

@cstyan

cstyan Sep 13, 2018

Contributor

wouldn't that just be whatever the value of ev.currentSamples is when we return?

This comment has been minimized.

@brian-brazil

brian-brazil Sep 18, 2018

Member

No, it'd also be additionally the number of samples we're returning.

This comment has been minimized.

@cstyan

cstyan Sep 18, 2018

Contributor

In every case? After the call to eval, we've potentially added to ev.currentSamples in eval via*Call, *VectorSelector cases, or via vectorSelector()/matrixSelector() in which cases we wouldn't have reset ev.currentSamples afterwards, or we've called rangeEval().

As I said earlier, I'm sill a bit lost understanding how to properly handle all of the cases. Any pointers would be helpful.

This comment has been minimized.

@brian-brazil

brian-brazil Sep 19, 2018

Member

Yes, in every case.

After the call to eval, we've potentially added to ev.currentSamples in ...

Yes, and that needs to be undone before we return as those are no longer in memory.

So if we start a rangeEval with say 10 samples used up by higher in the call stack, then any functions we call we will add to that - so say we're now at 30 samples. If the output of the rangeEval is 5 samples then currentSamples should be 15 by time the rangeEval returns.

This comment has been minimized.

@cstyan

cstyan Sep 19, 2018

Contributor

so then here before a call to ev.eval we want to be sure ev.currentSamples is saved to a temp var, and after the call ev.currentSamples should be set to that temp var plus the length of all the series in matrixes[i]?

This comment has been minimized.

@brian-brazil
@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Sep 13, 2018

Sorry @brian-brazil but I'm still a bit lost trying to understand exactly how the ev.currentSamples reset should work.

vectors[i] = append(vectors[i], Sample{Metric: series.Metric, Point: point})
// Move input vectors forward so we don't have to re-scan the same
// past points at the next step.
matrixes[i][si].Points = series.Points[1:]
ev.currentSamples++

This comment has been minimized.

@cstyan

cstyan Sep 20, 2018

Contributor

@brian-brazil so now I assume that since we're adding the # of samples in the matrices returned by the calls to eval, we don't need to add to ev.currentSamples here, but we should keep the later block where we add the length of the result from f. Is that correct?

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

No, we still need these as it is a (temporary) 2nd copy of some of the data in the input vector.


// In some cases we only have the result of 'f' without having tracked any samples
// in the above for loop that built the input vectors.
ev.currentSamples += len(result)

This comment has been minimized.

@cstyan

cstyan Sep 20, 2018

Contributor

keep this?

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

Yes, this will be part of the output

What you ultimately want is the original tempNumSamples plus all of these in ev.currentSamples when you return

for i, e := range exprs {
// Functions will take string arguments from the expressions, not the values.
if e != nil && e.Type() != ValueTypeString {
tempNumSamples = ev.currentSamples

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

This wipes out the tempNumSamples if there's more than one expr, you don't need this line

This comment has been minimized.

@cstyan

cstyan Sep 21, 2018

Contributor

I don't understand, each call to eval is adding samples to ev.currentSamples, but after the call we want the result of ev.currentSamples to be the value prior to the eval call plus the amount of samples returned.

This comment has been minimized.

@cstyan

cstyan Sep 21, 2018

Contributor

as we discussed here: #4513 (comment)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

After the call, the value of ev.currentSamples should already be the eval call plus the amount of samples returned - as that's the contract.

matrixes[i] = ev.eval(e).(Matrix)
ev.currentSamples = tempNumSamples + matrixes[i].TotalSamples()

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

ev.currentSamples will already have the right value due to the eval, no need to change it

@@ -700,16 +726,21 @@ func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ..
enh := &EvalNodeHelper{out: make(Vector, 0, biggestLen)}
seriess := make(map[uint64]Series, biggestLen) // Output series by series hash.
for ts := ev.startTimestamp; ts <= ev.endTimestamp; ts += ev.interval {
// Reset num. of samples in memory after each timestamp.
tempNumSamples = ev.currentSamples

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

You're losing the original value of tempNumSamples here

This comment has been minimized.

@brian-brazil

brian-brazil Sep 25, 2018

Member

You're still losing this here, you need a different variable to store this in.

This comment has been minimized.

@cstyan

cstyan Sep 25, 2018

Contributor

I see what you're saying. The number of samples we have going into the function is the value we want to ues (plus the amount of new samples) when returning, but we want to reset to the previous value of ev.currentSamples after each timestamp iteration. Latest commit should fix this.

vectors[i] = append(vectors[i], Sample{Metric: series.Metric, Point: point})
// Move input vectors forward so we don't have to re-scan the same
// past points at the next step.
matrixes[i][si].Points = series.Points[1:]
ev.currentSamples++

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

No, we still need these as it is a (temporary) 2nd copy of some of the data in the input vector.


// In some cases we only have the result of 'f' without having tracked any samples
// in the above for loop that built the input vectors.
ev.currentSamples += len(result)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 21, 2018

Member

Yes, this will be part of the output

What you ultimately want is the original tempNumSamples plus all of these in ev.currentSamples when you return

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Sep 21, 2018

Okay so then none of the changes in the latest commit are necessary?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Sep 24, 2018

No, those changes aren't needed.

The goal should be that at the end of the evalRange call (or any other eval call) that ev.currentSample is the number of samples that function is about to return plus the samples held by all calling functions up the call stack.

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from b04eefb to 33bce5a Sep 24, 2018

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Sep 24, 2018

Okay, I believe the latest commit is correct then.

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from 9c1b9ad to 79333ef Sep 25, 2018

keep track of samples per query, set a max # of samples that can be in
memory at once

Signed-off-by: Callum Stytan <callumstyan@gmail.com>
the value for resetting the number of samples after each timestamp
iteration and when returning from rangeEval are different and should be
treated as such

Signed-off-by: Callum Styan <callumstyan@gmail.com>

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from 79333ef to 1855ded Sep 25, 2018

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Sep 25, 2018

rebased off master and squashed everything but the lastest commit into one commit

@@ -705,16 +734,21 @@ func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ..
enh := &EvalNodeHelper{out: make(Vector, 0, biggestLen)}
seriess := make(map[uint64]Series, biggestLen) // Output series by series hash.
for ts := ev.startTimestamp; ts <= ev.endTimestamp; ts += ev.interval {
// Reset num. of samples in memory after each timestamp.
tempNumSamples = ev.currentSamples

This comment has been minimized.

@brian-brazil

brian-brazil Sep 26, 2018

Member

This can go above the for, and then you can reset it here - which is more natural than doing it at the end of the loop


// In some cases we only have the result of 'f' without having tracked any samples
// in the above for loop that built the input vectors.
ev.currentSamples += len(result)

This comment has been minimized.

@brian-brazil

brian-brazil Sep 26, 2018

Member

You want to add this to tempNumSamples as the output data from previous cycles is in memory.

This comment has been minimized.

@cstyan

cstyan Sep 26, 2018

Contributor

Hmmm, sorry I don't understand why.

In addition to, or instead of adding it to ev.currentSamples? It seems to me that it should be in addition to since we check ev.currentSamples when determining if we have too many samples in memory.

This comment has been minimized.

@brian-brazil

brian-brazil Sep 27, 2018

Member

Yes. After every iteration we're now holding the results of previous iterations in memory, which needs to be part of tempNumSamples.

cstyan added some commits Sep 26, 2018

reset current samples as first line of timestamp loop rather than last
line

Signed-off-by: Callum Styan <callumstyan@gmail.com>
tempNumSamples should also include the # of samples returned from
function f

Signed-off-by: Callum Styan <callumstyan@gmail.com>
@brian-brazil
Copy link
Member

brian-brazil left a comment

Core logic looks okay now

@@ -728,16 +763,29 @@ func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ..
ev.errorf("vector cannot contain metrics with the same labelset")
}
enh.out = result[:0] // Reuse result vector.

// In some cases we only have the result of 'f' without having tracked any samples

This comment has been minimized.

@brian-brazil

brian-brazil Sep 28, 2018

Member

I don't think this comment helps, one way or the other we should always be adding this.

This comment has been minimized.

@cstyan

cstyan Sep 28, 2018

Contributor

Yeah this comment is leftover from my initial go at implementing this, when I misunderstood what was happening in certain sections of rangeEval. I'll remove it.

}
}
// The seeked sample might also be in the range.
if ok {
t, v := it.Values()
if t == maxt && !value.IsStaleNaN(v) {
out = append(out, Point{T: t, V: v})
if ev.currentSamples < ev.maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Sep 28, 2018

Member

This should error


evalSpanTimer, ctx := q.stats.GetSpanTimer(ctx, stats.EvalTotalTime)
defer evalSpanTimer.Finish()
evalTimer := q.stats.GetTimer(stats.EvalTotalTime).Start()

This comment has been minimized.

@brian-brazil

brian-brazil Sep 28, 2018

Member

Why does the function change here?

This comment has been minimized.

@cstyan

cstyan Sep 28, 2018

Contributor

I think I picked the wrong change when fixing a merge conflict here during rebase, fixing.

cstyan added some commits Sep 28, 2018

revert an accidental change that happened during rebasing
Signed-off-by: Callum Styan <callumstyan@gmail.com>
remove a comment, fix max samples error handling in matrixIterSlice
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@brian-brazil
Copy link
Member

brian-brazil left a comment

Just some cleanup stuff.

for ts := ev.startTimestamp; ts <= ev.endTimestamp; ts += ev.interval {
// Reset num. of samples in memory after each timestamp.

This comment has been minimized.

@brian-brazil
// function call results.
func (ev *evaluator) rangeEval(f func([]Value, *EvalNodeHelper) Vector, exprs ...Expr) Matrix {
numSteps := int((ev.endTimestamp-ev.startTimestamp)/ev.interval) + 1
matrixes := make([]Matrix, len(exprs))
origMatrixes := make([]Matrix, len(exprs))

originalNumSamples := ev.currentSamples

This comment has been minimized.

@brian-brazil

brian-brazil Oct 1, 2018

Member

There's a bit much vertical whitespace here


originalNumSamples := ev.currentSamples

var tempNumSamples int

This comment has been minimized.

@brian-brazil

brian-brazil Oct 1, 2018

Member

Why not create this when you assign to it?

vectors[i] = append(vectors[i], Sample{Metric: series.Metric, Point: point})
// Move input vectors forward so we don't have to re-scan the same
// past points at the next step.
matrixes[i][si].Points = series.Points[1:]
ev.currentSamples++
} else if ev.currentSamples >= ev.maxSamples {

This comment has been minimized.

@brian-brazil

brian-brazil Oct 1, 2018

Member

The else here and duplicated inverse condition is a bit odd, it seems like it might be accidentally broken bu a future change. I'd move this inside the above if as a sub if.

return mat
}
// Add samples in output vector to output series.

This comment has been minimized.

@brian-brazil

brian-brazil Oct 1, 2018

Member

This should stay

@@ -828,6 +878,7 @@ func (ev *evaluator) eval(expr Expr) Value {
if i != matrixArgIndex {
otherArgs[i] = ev.eval(e).(Matrix)
otherInArgs[i] = Vector{Sample{}}
// (callum) this is just building args for later procesing so I don't think we need to track samples here

This comment has been minimized.

@brian-brazil

brian-brazil Oct 1, 2018

Member

Can you either clean these up or make them proper comments?

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Oct 1, 2018

removed most of the comments you pointed out as they were only necessary while I was trying to track where samples were loaded in and out of memory, updated the rest and simplified the if block

cleanup, mostly removing comments and simplifying an if block
Signed-off-by: Callum Styan <callumstyan@gmail.com>

@cstyan cstyan force-pushed the cstyan:callum-4414 branch from 2742b9c to b5e0f32 Oct 1, 2018

@brian-brazil brian-brazil merged commit 9bca041 into prometheus:master Oct 2, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Oct 2, 2018

Thanks!

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Oct 2, 2018

@brian-brazil thanks for all the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment