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

Add present_over_time #9097

Merged
merged 6 commits into from
Jul 29, 2021
Merged

Add present_over_time #9097

merged 6 commits into from
Jul 29, 2021

Conversation

darshanime
Copy link
Contributor

Signed-off-by: darshanime deathbullet@gmail.com

closes #8255

Signed-off-by: darshanime <deathbullet@gmail.com>
@@ -126,7 +126,7 @@ func (i *isolation) State() *isolationState {

// newAppendID increments the transaction counter and returns a new transaction
// ID. The first ID returned is 1.
// Also returns the low watermark, to keep lock/unlock operations down
// Also returns the low watermark, to keep lock/unlock operations down.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -530,8 +530,6 @@ func (ng *Engine) exec(ctx context.Context, q *query) (v parser.Value, ws storag
// Cancel when execution is done or an error was raised.
defer q.cancel()

const env = "query execution"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

already defined here

env = "query execution"

Signed-off-by: darshanime <deathbullet@gmail.com>
@darshanime darshanime marked this pull request as ready for review July 18, 2021 11:48
Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request. It seems that this is a complex approach, I would implement it like other functions:

func funcPresentOverTime(vals []parser.Value, args parser.Expressions, enh *EvalNodeHelper) Vector {
        return aggrOverTime(vals, enh, func(values []Point) float64 {
                return 1
        })
}

Signed-off-by: darshanime <deathbullet@gmail.com>
@roidelapluie
Copy link
Member

Thanks! I will let a few more days for @juliusv to chime in.

@@ -513,6 +513,15 @@ func funcAbsentOverTime(vals []parser.Value, args parser.Expressions, enh *EvalN
})
}

// === present_over_time(Vector parser.ValueTypeMatrix) Vector ===
// This function will return 1 if the matrix has at least one element.
// Due to engine optimization, this function is only called when this condition is true.
Copy link
Member

Choose a reason for hiding this comment

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

I believe this copied comment only applies to absent_over_time(), for which this engine optimization exists:

prometheus/promql/engine.go

Lines 1366 to 1402 in 7337ecf

// The absent_over_time function returns 0 or 1 series. So far, the matrix
// contains multiple series. The following code will create a new series
// with values of 1 for the timestamps where no series has value.
if e.Func.Name == "absent_over_time" {
steps := int(1 + (ev.endTimestamp-ev.startTimestamp)/ev.interval)
// Iterate once to look for a complete series.
for _, s := range mat {
if len(s.Points) == steps {
return Matrix{}, warnings
}
}
found := map[int64]struct{}{}
for i, s := range mat {
for _, p := range s.Points {
found[p.T] = struct{}{}
}
if i > 0 && len(found) == steps {
return Matrix{}, warnings
}
}
newp := make([]Point, 0, steps-len(found))
for ts := ev.startTimestamp; ts <= ev.endTimestamp; ts += ev.interval {
if _, ok := found[ts]; !ok {
newp = append(newp, Point{T: ts, V: 1})
}
}
return Matrix{
Series{
Metric: createLabelsForAbsentFunction(e.Args[0]),
Points: newp,
},
}, warnings
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I thought it was appropriate since both the functions are called only when the corresponding data exists.

outVec := call(inArgs, e.Args, enh)

Removed...

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, you were right, sorry. Still fine to omit that line I think, it's normal for any _over_time() function to only be called when there's data for it :)

For absent_over_time() it probably just made more sense to call it out because the function returns something even (and only if) it was not normally called.

promql/functions.go Outdated Show resolved Hide resolved
@juliusv
Copy link
Member

juliusv commented Jul 20, 2021

Thanks! Could you add a documentation entry for the function in the same PR, in https://github.com/prometheus/prometheus/blob/main/docs/querying/functions.md#aggregation_over_time?

Signed-off-by: darshanime <deathbullet@gmail.com>
@roidelapluie
Copy link
Member

The documentation should be a bullet in the over time https://prometheus.io/docs/prometheus/latest/querying/functions/#aggregation_over_time

Signed-off-by: darshanime <deathbullet@gmail.com>
@@ -448,6 +430,7 @@ over time and return an instant vector with per-series aggregation results:
* `stddev_over_time(range-vector)`: the population standard deviation of the values in the specified interval.
Copy link
Member

Choose a reason for hiding this comment

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

the value 1 for any series in the specified interval.

I think it would be more clear, WDYT?

Signed-off-by: darshanime <deathbullet@gmail.com>
@juliusv
Copy link
Member

juliusv commented Jul 29, 2021

👍 Thank you :)

@juliusv juliusv merged commit c4f2e9e into prometheus:main Jul 29, 2021
@darshanime darshanime deleted the present_over_time branch July 29, 2021 11:05
@roidelapluie roidelapluie mentioned this pull request Jul 29, 2021
valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this pull request Aug 3, 2021
valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this pull request Aug 3, 2021
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.

Add <aggregation>_over_time function that always returns 1
3 participants