[BUGFIX] Fix tree view broken when using prometheus built-in vars #2405
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AntoineThebaud
commented
Nov 7, 2024
| const queryExpr = useReplaceVariablesInString(rest.value); | ||
| let queryExpr = useReplaceVariablesInString(rest.value); | ||
| if (queryExpr) { | ||
| queryExpr = replacePromBuiltinVariables(queryExpr, 12345, 12345); // TODO placeholder values to replace |
There was a problem hiding this comment.
Looks like providing proper values for the step parameters here involves some refactoring: currently I'd need to rely on the timeseries query context, but actually these step values are computed independently / before the queries are getting fired, so maybe we should extract them to independant hooks to be reused here? Need to think about it...
Signed-off-by: Antoine THEBAUD <antoine.thebaud@yahoo.fr>
Signed-off-by: Antoine THEBAUD <antoine.thebaud@yahoo.fr>
66cc498 to
1183060
Compare
… issue.. Signed-off-by: Antoine THEBAUD <antoine.thebaud@yahoo.fr>
1183060 to
26e3b29
Compare
AntoineThebaud
commented
Nov 21, 2024
| @@ -11,8 +11,6 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| import { replaceVariable } from '@perses-dev/plugin-system'; | |||
There was a problem hiding this comment.
This import was causing an issue with Jest:
FAIL src/utils/utils.test.ts
● Test suite failed to run
TypeError: Cannot read properties of undefined (reading 'install')
24 | import { getFormattedAxis } from '../utils';
25 |
> 26 | use([EChartsBarChart, GridComponent, DatasetComponent, TitleComponent, TooltipComponent, CanvasRenderer]);
| ^
27 |
28 | const BAR_WIN_WIDTH = 14;
29 | const BAR_GAP = 6;
at use (../node_modules/echarts/lib/extension.js:109:7)
at ../node_modules/echarts/lib/extension.js:96:7
at Array.forEach (<anonymous>)
at each (../node_modules/echarts/node_modules/zrender/lib/core/util.js:205:13)
at use (../node_modules/echarts/lib/extension.js:95:5)
at Object.<anonymous> (../components/src/BarChart/BarChart.tsx:26:4)
at Object.<anonymous> (../components/src/BarChart/index.ts:17:14)
at Object.<anonymous> (../components/src/index.ts:18:14)
at Object.<anonymous> (../plugin-system/src/components/CalculationSelector/CalculationSelector.tsx:24:21)
at Object.<anonymous> (../plugin-system/src/components/CalculationSelector/index.ts:17:14)
at Object.<anonymous> (../plugin-system/src/components/index.ts:17:14)
at Object.<anonymous> (../plugin-system/src/index.ts:17:14)
at Object.<anonymous> (src/utils/utils.ts:37:23)
at Object.<anonymous> (src/utils/utils.test.ts:17:16)
This was due to the fact that Jest doesn't support tree shaking, about that see
- can jest support tree-shaking jestjs/jest#11230
- https://dev.to/fogel/potential-issues-with-barrel-files-in-jest-1nkl
The location change here is just a way to work around the issue, it's actually surprising that we didnt face it earlier already..
Nexucis
approved these changes
Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
With #2344 I missed the fact that prometheus-specific builtin variables (
$__interval,$__rate_interval..) aren't getting replaced by proper values with the current logic.. Thus any query containing such variable is producing an error (prometheus cannot parse this perses-specific syntax). This PR aims to fix that.Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes
See Storybook
and e2e docs for more details. Common issues
include: