feat: updated the logic for variable update queue #6586
Conversation
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
2 similar comments
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to fa4aeae in 1 minute and 38 seconds
More details
- Looked at
307lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:33
- Draft comment:
ThegetDependentVariablesfunction is duplicated in multiple files. Consider defining it in a single location and importing it where needed to avoid redundancy and potential inconsistencies. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative because it assumes the function is duplicated in other files, but the diff does not provide evidence of this. Without evidence, the comment does not meet the criteria for a necessary code change.
I might be missing context from other files that are not shown in the diff. The function could indeed be duplicated elsewhere, but I have no way to verify this from the current information.
Given the rules, I should only consider the information presented in the diff. Without evidence of duplication in the diff, the comment should be removed.
Remove the comment because it is speculative and not based on evidence from the diff.
2. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:99
- Draft comment:
ThevalidVariableUpdatefunction's logic for checking if the component is mounted is inverted. It should return true if the component is mounted and the variable is in the update queue, not the other way around. Consider revising the logic. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:95
- Draft comment:
TheuseEffectthat setsisMounted.currentto true is missing a dependency array. Add an empty dependency array to ensure this effect only runs once when the component mounts. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:33
-
Draft comment:
This function is duplicated. Consider importing and reusing the existing implementation instead. -
function
getDependentVariables(VariableItem.tsx) -
Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_so46fFL9ZJXaSHHK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
I merged the three files based on version 0.57.0 of the code, and the query interface worked, but the query_range interface reported an error. |
|
@cxposition - Hey, it seems like there is some issue with composite queries, and it seems like you are missing configs there, so can you please share the URL of this above scenario? It will help with the debugging. |
I'm sorry that the company's Intranet environment cannot be provided, maybe it is our configuration problem, we will investigate and solve it first, and I will share with you as soon as there is any progress, thank you for your reply. |
The problem is resolved. It is a configuration problem.Thanks for your reply. |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
| @@ -21,6 +26,11 @@ function DashboardVariableSelection(): JSX.Element | null { | |||
|
|
|||
| const [variablesTableData, setVariablesTableData] = useState<any>([]); | |||
|
|
|||
| const [dependencyData, setDependencyData] = useState<{ | |||
| order: string[]; | |||
| graph: VariableGraph; | |||
There was a problem hiding this comment.
extract out to independent type , IDependencyData
| const initializationRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| if (variablesTableData.length > 0 && !initializationRef.current) { |
There was a problem hiding this comment.
why is this required ?
- since the initial state of dependency data is null, a null check would be sufficient to know the first load ?
- on similar lines we do not want to re-calculate the deps graph / order when
variablesTableDatais updated ? when adding or removing variables from the dashboard ?
There was a problem hiding this comment.
done, removed ref and added a check on the dependencyGraph as discussed
There was a problem hiding this comment.
Also tested variable addition and deletion
| @@ -88,18 +89,26 @@ function VariableItem({ | |||
| (state) => state.globalTime, | |||
| ); | |||
|
|
|||
| // logic to detect if its a rerender or a new render/mount | |||
| const isMounted = useRef(false); | |||
There was a problem hiding this comment.
why is onMount handling different ? onMount should be handled via the root triggers ?
There was a problem hiding this comment.
removed and added root trigger handling
| @@ -29,3 +29,110 @@ export const convertVariablesToDbFormat = ( | |||
| result[id] = obj; | |||
| return result; | |||
| }, {}); | |||
|
|
|||
| const getDependentVariables = (queryValue: string): string[] => { | |||
| const variableRegexPattern = /\{\{\s*?\.([^\s}]+)\s*?\}\}/g; | |||
There was a problem hiding this comment.
we do support a bunch of other variable formats as well, please do include them as well
| } | ||
|
|
||
| if (topologicalOrder.length !== Object.keys(dependencies).length) { | ||
| throw new Error('Cycle detected in the dependency graph!'); |
There was a problem hiding this comment.
we want to break the UI here ? maybe show some user consumable error here ? or detect this while saving the variables so that this doesn't occur ?
There was a problem hiding this comment.
Added console.error - to help debugging and created a ticket to work on a better informative UI for this -
#6715
| allSelected, | ||
| isMounted.current, | ||
| ); | ||
| setVariablesToGetUpdated((prev) => |
There was a problem hiding this comment.
onValueUpdate should be responsible to remove the variable from variablesToGetUpdated ?
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
… variables (#6609) * feat: added API limiting to reduce unneccesary api call for dashboard variables * feat: fixed dropdown open triggering the api calls for single-select and misc
…sors - dashboardVariables (#6621) * feat: added API limiting to reduce unneccesary api call for dashboard variables * feat: fixed dropdown open triggering the api calls for single-select and misc * feat: add jest test cases for new logic's utils, functions and processors - dashboardVariables * feat: added test for checkAPIInvocation * feat: refactor code * feat: added more test on graph utilities
SagarRajput-7
force-pushed
the
variable-update-queue
branch
from
1abf809 to
5097375
Compare
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 5097375 in 32 seconds
More details
- Looked at
518lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:49
- Draft comment:
Remove the console.log statement to clean up the code.
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_opWqhmMAiMrrkLH0
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| export const buildDependencies = ( | ||
| variables: IDashboardVariable[], | ||
| ): VariableGraph => { | ||
| console.log('buildDependencies', variables); |
There was a problem hiding this comment.
Remove the console.log statement to avoid unnecessary console output.
| console.log('buildDependencies', variables); |
|
@SagarRajput-7 is this point about not resetting already selected values https://www.notion.so/signoz/Dashboard-Variables-8783d02224c0483b9e8a4b8218cb5ddc?pvs=4#144fcc6bcd1980dda2c2ede018f29914. Is it part of this PR? or is it yet to be implemented? |
|
Thanks for pointing this, fixed in the latest commit - Screen.Recording.2024-12-26.at.10.39.21.AM.mov |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f2bb467 in 19 seconds
More details
- Looked at
284lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:115
- Draft comment:
Consider using a more robust error handling mechanism instead ofconsole.errorfor cycle detection in the dependency graph. - Reason this comment was not posted:
Confidence changes required:50%
The error message for cycle detection in the dependency graph is logged usingconsole.error, which is appropriate for debugging but might not be suitable for production environments. Consider using a more robust error handling mechanism.
2. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:171
- Draft comment:
Revisit the logic incheckAPIInvocationwhere it returnsfalseifparentDependencyGraphis empty. Ensure this aligns with the intended behavior for variables without dependencies. - Reason this comment was not posted:
Confidence changes required:50%
ThecheckAPIInvocationfunction has a condition that returnsfalseifparentDependencyGraphis empty. This might lead to unexpected behavior if the graph is supposed to be empty for certain variables. Consider revisiting this logic.
3. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:175
- Draft comment:
Revisit the logic incheckAPIInvocationwhere it returnstrueif there are no dependencies. Ensure this aligns with the intended behavior for API invocation checks. - Reason this comment was not posted:
Confidence changes required:50%
ThecheckAPIInvocationfunction has a condition that returnstrueif there are no dependencies. This might not be the intended behavior if the function is supposed to check for dependencies before invocation. Consider revisiting this logic.
Workflow ID: wflow_1dZFStJAISA2kCLX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
No, this is part of the |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1ee2cc7 in 35 seconds
More details
- Looked at
126lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:40
- Draft comment:
The regex pattern ingetDependentVariablesis complex and could be simplified for better readability. Consider breaking it down or adding comments to explain each part. - Reason this comment was not posted:
Confidence changes required:50%
The functiongetDependentVariablesuses a regex pattern to match variable names in different formats. However, the regex pattern is complex and could be simplified for better readability and maintainability. Additionally, the function uses a map to extract variable names, which could be optimized.
2. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:34
- Draft comment:
The regex pattern ingetDependentVariablesis complex and could be simplified for better readability. Consider breaking it down or adding comments to explain each part. - Reason this comment was not posted:
Confidence changes required:50%
ThegetDependentVariablesfunction uses a regex pattern to match variable names in different formats. However, the regex pattern is complex and could be simplified for better readability and maintainability. Additionally, the function uses a map to extract variable names, which could be optimized.
3. frontend/src/container/NewDashboard/DashboardVariablesSelection/__test__/dashboardVariables.test.tsx:106
- Draft comment:
The test case expectscheckAPIInvocationto return true whenparentDependencyGraphis empty, but the implementation returns false. Verify the logic incheckAPIInvocation. - Reason this comment was not posted:
Comment did not seem useful.
4. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx:49
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to all similar instances in this file. - Reason this comment was not posted:
Comment was on unchanged code.
5. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx:70
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to all similar instances in this file. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx:90
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to all similar instances in this file. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx:124
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to all similar instances in this file. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx:153
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to all similar instances in this file. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_DZXcrgw61mV7PQPe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Summary
Under this change I have introduced the following things -
For more info refer to the
Technical Analysis and new approachblock of this notion doc - https://www.notion.so/signoz/Dashboard-Variables-8783d02224c0483b9e8a4b8218cb5ddc?pvs=4#2c0617c157a141f5a09ba261d9d15d4dTest cases -
Related Issues / PR's
Screenshots
Comparison between local and prod env
Screen.Recording.2024-12-04.at.10.15.42.AM.mov
Affected Areas and Manually Tested Areas
Tested before and after of -
Important
Replaces variable update queue with a dependency graph in dashboard variable management, adding new utility functions and tests.
DashboardVariableSelection.tsx.onValueUpdateto handle mounted and non-mounted calls.onVarChangedfunction.buildDependencies,buildDependencyGraph,buildParentDependencyGraph, andonUpdateVariableNodeinutil.ts.validVariableUpdatelogic inVariableItem.tsx.dashboardVariables.test.tsx.useReffor initialization checks inDashboardVariableSelection.tsxandVariableItem.tsx.VariableItem.tsx.This description was created by
for 1ee2cc7. It will automatically update as commits are pushed.