-
Notifications
You must be signed in to change notification settings - Fork 54
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
add absent function #172
Conversation
Signed-off-by: siddhikhapare <[email protected]>
@fpetkovski Can you review it once? If any changes required please let me know. |
engine/engine_test.go
Outdated
load: `load 30s | ||
absent{pod="nginx-1", series="1"} 0 | ||
absent{pod="nginx-2", series="1"} 0`, | ||
query: "absent(absent)", |
There was a problem hiding this comment.
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
engine/engine_test.go
Outdated
load: `load 30s | ||
absent{pod="nginx-1", series="1"} 0 | ||
absent{pod="nginx-2", series="1"} 0`, | ||
query: "absent(absent)", |
There was a problem hiding this comment.
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 <[email protected]>
Signed-off-by: siddhikhapare <[email protected]>
@yeya24 I have made changes as you suggested. Can you review it. Thank you in advanced. |
Please include 2 test cases for And please fix the test, test failure seems related. |
Signed-off-by: siddhikhapare <[email protected]>
@yeya24 I fixed errors. Can you please review it? Thank you. |
Looks like tests are still failing. You can run |
ok |
`
}, |
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 <[email protected]>
@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 -
In TestInstantQuery -
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- testutil.go:91: engine_test.go:2586: "" === RUN TestInstantQuery/disableOptimizers=false/absent_with_non- testutil.go:91: engine_test.go:2586: "" === RUN TestInstantQuery/disableOptimizers=false/absent_with_present_metric_and_label#01 |
@yeya24 I am new to this project. I am exploring it. Thank you. |
@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)", |
There was a problem hiding this comment.
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)", |
There was a problem hiding this comment.
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{}, | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: siddhikhapare [email protected]
Fixes: #138