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 absent function #172

Closed
wants to merge 5 commits into from
Closed

add absent function #172

wants to merge 5 commits into from

Conversation

siddhikhapare
Copy link

@siddhikhapare siddhikhapare commented Feb 9, 2023

Signed-off-by: siddhikhapare siddhikhapare66@bvcoenm.edu.in

Fixes: #138

Signed-off-by: siddhikhapare <siddhikhapare66@bvcoenm.edu.in>
@siddhikhapare
Copy link
Author

@fpetkovski Can you review it once? If any changes required please let me know.

load: `load 30s
absent{pod="nginx-1", series="1"} 0
absent{pod="nginx-2", series="1"} 0`,
query: "absent(absent)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another test case where the metric doesn't exist? Also let's rename the metric name to not be absent

load: `load 30s
absent{pod="nginx-1", series="1"} 0
absent{pod="nginx-2", series="1"} 0`,
query: "absent(absent)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Signed-off-by: siddhikhapare <siddhikhapare66@bvcoenm.edu.in>
Signed-off-by: siddhikhapare <siddhikhapare66@bvcoenm.edu.in>
@siddhikhapare
Copy link
Author

@yeya24 I have made changes as you suggested. Can you review it. Thank you in advanced.

@yeya24
Copy link
Contributor

yeya24 commented Feb 11, 2023

Please include 2 test cases for absent. One is calling absent when the queried series is present, one is not. I see you only have the case of absent.

And please fix the test, test failure seems related.

Signed-off-by: siddhikhapare <siddhikhapare66@bvcoenm.edu.in>
@siddhikhapare
Copy link
Author

@yeya24 I fixed errors. Can you please review it? Thank you.

@fpetkovski
Copy link
Collaborator

Looks like tests are still failing. You can run make test to reproduce the failures.

@siddhikhapare
Copy link
Author

ok

@siddhikhapare
Copy link
Author

siddhikhapare commented Feb 16, 2023

`
"sort": func(f FunctionArgs) promql.Sample {
if len(f.Points) == 0 {
return InvalidSample
}

sortedPoints := make([]promql.Point, len(f.Points))
copy(sortedPoints, f.Points)
sort.Slice(sortedPoints, func(i, j int) bool {
	return sortedPoints[i].V < sortedPoints[j].V
})

sortedValues := make([]float64, len(sortedPoints))
for i, point := range sortedPoints {
	sortedValues[i] = point.V
}

return promql.Sample{
	Metric: f.Labels,
	Point: promql.Point{
		T: f.StepTime,
		V: sortedValues,
	},
}

},
`
@fpetkovski @yeya24 In this implementation of sort, sortedPoints is of type []promql.Point and sortedValues is of type []float64. In final return statement, the V field of promql.Point is assigned a []float64 type which is not allowed as V is of type float64. To solve this issue, I modified Point struct to have a slice of float64 instead of a single float64 value for V but did not work. I do not understand while testing it provide me error as can't use sortedValues (variable of type []float64) as type float64 in struct literal. Can you give me some pointers on this?

@yeya24
Copy link
Contributor

yeya24 commented Feb 16, 2023

I prefer to not modify the existing struct but have a separate operator to deal with sort and sort desc.

Let's focus on absent in this pr and fix tests first.

Signed-off-by: siddhikhapare <siddhikhapare66@bvcoenm.edu.in>
@siddhikhapare
Copy link
Author

siddhikhapare commented Feb 19, 2023

@yeya24 According prometheus query function documentation I have done changes in test cases. absent function used for alerting on when no time series exist for a given metric name and label combination.

In TestQueriesAgainstOldEngine -

{
    name: "absent with non-present metric",
    load: "",
    query: "absent(dummy_metric)",
    start: time.Unix(0, 0),
    end: time.Unix(3000, 0),
    step: 10 * time.Second,
},
{
    name: "absent with non-present label",
    load: `load 30s
        http_requests_total{pod="nginx-1", series="1"} 1+1.1x40
        http_requests_total{pod="nginx-2", series="1"} -10+1x50`,
    query: "absent(http_requests_total{pod=\"nginx-3\"})",
    start: time.Unix(0, 0),
    end: time.Unix(3000, 0),
    step: 10 * time.Second,
},
{
    name: "absent with present metric and label",
    load: `load 30s
        http_requests_total{pod="nginx-1", series="1"} 1+1.1x40
        http_requests_total{pod="nginx-2", series="1"} -10+1x50`,
    query: "absent(http_requests_total{pod=\"nginx-1\"})",
    start: time.Unix(0, 0),
    end: time.Unix(3000, 0),
    step: 10 * time.Second,
},

In TestInstantQuery -

{
    name: "absent with non-present metric",
    load: "",
    query: "absent(dummy_metric)",
    queryTime: time.Unix(50, 0),
    sortByLabels: true,
},
{
    name: "absent with non-present label",
    load: `load 30s
        http_requests_total{pod="nginx-1", series="1"} 1+1.1x40
        http_requests_total{pod="nginx-2", series="1"} -10+1x50`,
    query: "absent(http_requests_total{pod=\"nginx-3\"})",
    queryTime: time.Unix(50, 0),
    sortByLabels: true,
},
{
    name: "absent with present metric and label",
    load: `load 30s
        http_requests_total{pod="nginx-1", series="1"} 1+1.1x40
        http_requests_total{pod="nginx-2", series="1"} -10+1x50`,
    query: "absent(http_requests_total{pod=\"nginx-1\"})",
    queryTime: time.Unix(50, 0),
    sortByLabels: true,
},

But while running test found errors which indicate that the expected and actual results are not matching for the tests. Can you help me to understand what are my mistakes to fixed errors. I mentioned my errors are -

RUN TestInstantQuery/disableOptimizers=false/absent_with_non-
present_metric#01/disableFallback=false

testutil.go:91: engine_test.go:2586: ""
exp: &promql.Result{Err:error(nil),
Value:promql.Vector{promql.Sample{Point:promql.Point{T:50000, V:1, H:(*histogram.FloatHistogram)
(nil)}, Metric:labels.Labels{}}}, Warnings:storage.Warnings(nil)}
got: &promql.Result{Err:error(nil), Value:promql.Vector{}, Warnings:storage.Warnings(nil)}
Diff:
--- Expected
+++ Actual
@@ -1,2 +1,2 @@
-(*promql.Result)({} => 1 @[50000])
+(*promql.Result)()

=== RUN TestInstantQuery/disableOptimizers=false/absent_with_non-
present_label#01/disableFallback=true

testutil.go:91: engine_test.go:2586: ""
exp: &promql.Result{Err:error(nil),
Value:promql.Vector{promql.Sample{Point:promql.Point{T:50000, V:1, H:(*histogram.FloatHistogram)
(nil)}, Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-3"}}}},
Warnings:storage.Warnings(nil)}
got: &promql.Result{Err:error(nil), Value:promql.Vector{}, Warnings:storage.Warnings(nil)}
Diff:
--- Expected
+++ Actual
@@ -1,2 +1,2 @@
-(*promql.Result)({pod="nginx-3"} => 1 @[50000])
+(*promql.Result)()

=== RUN TestInstantQuery/disableOptimizers=false/absent_with_present_metric_and_label#01
=== RUN
TestInstantQuery/disableOptimizers=false/absent_with_present_metric_and_label#01/disableFallback=false
testutil.go:91: engine_test.go:2586: ""
exp: &promql.Result{Err:error(nil), Value:promql.Vector{}, Warnings:storage.Warnings(nil)}
got: &promql.Result{Err:error(nil),
Value:promql.Vector{promql.Sample{Point:promql.Point{T:50000, V:0, H:(*histogram.FloatHistogram)
(nil)}, Metric:labels.Labels{labels.Label{Name:"pod", Value:"nginx-1"}, labels.Label{Name:"series",
Value:"1"}}}}, Warnings:storage.Warnings(nil)}
Diff:
--- Expected
+++ Actual
@@ -1,2 +1,2 @@
-(*promql.Result)()
+(*promql.Result)({pod="nginx-1", series="1"} => 0 @[50000])

@siddhikhapare
Copy link
Author

@yeya24 I am new to this project. I am exploring it. Thank you.

@yeya24
Copy link
Contributor

yeya24 commented Feb 19, 2023

@siddhikhapare Please use the debugger to see how the engine is implemented and that could help you understand how to fix it.

load: `load 30s
absent{pod="nginx-1", series="1"} 0
absent{pod="nginx-2", series="1"} 0`,
query: "absent(metric)",
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case seems wrong itself. Your metric name is absent but you are calling absent for metric. metric is not present in your test data.

load: `load 30s
absent{pod="nginx-1", series="1"} 0
absent{pod="nginx-2", series="1"} 0`,
query: "absent(metric)",
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

return promql.Sample{
Metric: f.Labels,
Point: promql.Point{},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the correct. absent checks whether the series exists ot not, not samples. When the series exists, I think what you want is to drop the whole series, not returning an empty point. Implementing a function like this couldn't achieve the goal of dropping the series right now, you have to refactor the framework to do that.

Probably a dedicated absent operator is easier.

Copy link
Author

Choose a reason for hiding this comment

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

@yeya24 Thank you for the guidance. I will go through @MichaHoffmann implementation to understand it.

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 support for all functions
3 participants