Add holt_winters backwards compatibility in Cortex parser#7223
Add holt_winters backwards compatibility in Cortex parser#7223afhassan wants to merge 3 commits intocortexproject:masterfrom
Conversation
yeya24
left a comment
There was a problem hiding this comment.
Can we also refactor the code a little bit? The same code exists in
Line 723 in caedb08
We should try to consolidate the code. It sounds a miss of not adding holt winters here when we added the cortex parser module. If that's the case we should remove the same code above.
pkg/parser/parser.go
Outdated
| holtWinters.Experimental = false | ||
| holtWinters.Name = "holt_winters" | ||
| fns["holt_winters"] = &holtWinters | ||
| promql.FunctionCalls["holt_winters"] = promql.FunctionCalls["double_exponential_smoothing"] |
There was a problem hiding this comment.
Can you add a test?
Also I am trying to understand why our integration test didn't capture this issue before. https://github.com/cortexproject/cortex/blob/master/integration/backward_compatibility_test.go#L161
There was a problem hiding this comment.
Also I think this should be behind a feature flag as well. Same as
Line 723 in caedb08
There was a problem hiding this comment.
Refactored it to register holt_winters for both parsers in the same place with a flag set to true.
The reason backward_compatibility_test did not catch the issue is that cortexparser was never used in it. We use it for validation in query-frontend, but there were no flags set that use cortexparser for validation. I added max_query_limit and that triggered the error.
69626e9 to
5ca20c5
Compare
Signed-off-by: Ahmed Hassan <[email protected]>
Signed-off-by: Ahmed Hassan <[email protected]>
Signed-off-by: Ahmed Hassan <[email protected]>
5ca20c5 to
f4ab757
Compare
What this PR does:
Cortex supports
holt_winters()for backwards compatibility. However this behaviour is currently broken as the function is only added to the engine parser at runtime and is not added tocortex_parserwhen it is initialized, causing queries to fail with error:Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]