-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: removed caching for all other panel type for expression except time series #6720
Conversation
…om graph in v3 | 6484
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
Aniket seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
👍 Looks good to me! Reviewed everything up to 9fb7160 in 24 seconds
More details
- Looked at
14
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/query-service/app/queryBuilder/query_builder.go:444
- Draft comment:
The conditionif params.Version != "v4" && params.CompositeQuery.PanelType != v3.PanelTypeGraph
is used to prevent caching for non-graph panel types in v3. Ensure this logic is consistently applied across all relevant sections of the codebase where caching decisions are made. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/query-service/app/queryBuilder/query_builder.go:441
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_kcYtJwZHBSWRlKEG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@aniketio-ctrl, please add test and sign cla
|
Added the testcase please check |
…om graph in v3 | 6484
Summary
Issue: Wrong caching for metric expressions apart from panel type graph in v3
According to current implementation , we are not caching queries, we are only caching expressions, we are caching if the panel type is graph, so if we are not caching single value panel type queries, we should not also be caching the result of the expression.
for eg-
If the expression is 'A+B', caching key would be '+' that dosen't consider the fact that wether A or B were same or not.
So while creating caching keys for expression , added a check if the version is not v4 then don't proceed unless its panelType: Graph
Related Issues / PR's
Fixes #6484
Screenshots
We are getting cached result with actual result which are vague
Affected Areas and Manually Tested Areas
Manually tested Aread ->
1.) Api -> /api/v3/query_range, tested with different panel type and checking wether we were getting correct result or not
Important
Fix caching behavior in
GenerateKeys
ofquery_builder.go
to skip non-graph panel types in non-v4 versions.GenerateKeys
ofquery_builder.go
, skip caching for expressions if version is not v4 and panel type is notGraph
.Graph
panel type in non-v4 versions to prevent incorrect cache results for other panel types.This description was created by for 9fb7160. It will automatically update as commits are pushed.