feat: updated the logic for variable update queue #6586

SagarRajput-7 · 2024-12-04T05:06:26Z

Summary

Under this change I have introduced the following things -

A new approach using topological sort to replace the current variable queue approach, which contains many issues and redundancy
With the new approach, multiple customer issues around the dashboard not loading are fixed. (issues mentioned below)

For more info refer to the Technical Analysis and new approach block of this notion doc - https://www.notion.so/signoz/Dashboard-Variables-8783d02224c0483b9e8a4b8218cb5ddc?pvs=4#2c0617c157a141f5a09ba261d9d15d4d

Related Issues / PR's

Screenshots

Comparison between local and prod env

Staging Env - (No throttling) -

Screen.Recording.2024-12-04.at.10.15.42.AM.mov

Affected Areas and Manually Tested Areas

Tested before and after of -

API sequencing, multiselect - single select behaviour
tried multiple reload to ensure correct loads
made a dashboard to test the dependency updates - https://stagingapp.signoz.io/dashboard/80125672-1709-4b19-aa41-30b188395b06?relativeTime=30m
Tested customer issues around dashboard loading (mentioned above and also see the videos added)

Important

Replaces variable update queue with a dependency graph in dashboard variable management, adding new utility functions and tests.

Behavior:
- Replaces variable update queue with dependency graph in DashboardVariableSelection.tsx.
- Updates onValueUpdate to handle mounted and non-mounted calls.
- Removes onVarChanged function.
Functions:
- Adds buildDependencies, buildDependencyGraph, buildParentDependencyGraph, and onUpdateVariableNode in util.ts.
- Updates validVariableUpdate logic in VariableItem.tsx.
Tests:
- Adds tests for new utility functions in dashboardVariables.test.tsx.
Misc:
- Adds useRef for initialization checks in DashboardVariableSelection.tsx and VariableItem.tsx.
- Minor refactoring in VariableItem.tsx.

^{This description was created by}^{for 5097375. It will automatically update as commits are pushed.}

github-actions · 2024-12-04T05:06:39Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

github-actions · 2024-12-04T05:06:51Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

github-actions · 2024-12-04T05:08:09Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

❌ Changes requested. Reviewed everything up to fa4aeae in 1 minute and 38 seconds

More details

Looked at 307 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 4 drafted comments based on config settings.

1. frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts:33

Draft comment:
The getDependentVariables function 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:
The validVariableUpdate function'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:
The useEffect that sets isMounted.current to 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.

frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts

github-actions · 2024-12-04T05:15:35Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

cxposition · 2024-12-05T06:07:45Z

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.

SagarRajput-7 · 2024-12-05T06:39:28Z

@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.

cxposition · 2024-12-05T07:18:28Z

@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.

cxposition · 2024-12-06T07:24:39Z

@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.

The problem is resolved. It is a configuration problem.Thanks for your reply.

github-actions · 2024-12-17T12:00:49Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

vikrantgupta25 · 2024-12-17T12:11:15Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx

@@ -21,6 +26,11 @@ function DashboardVariableSelection(): JSX.Element | null {

 	const [variablesTableData, setVariablesTableData] = useState<any>([]);

+	const [dependencyData, setDependencyData] = useState<{
+		order: string[];
+		graph: VariableGraph;


extract out to independent type , IDependencyData

vikrantgupta25 · 2024-12-17T12:16:01Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx

+	const initializationRef = useRef(false);
+
+	useEffect(() => {
+		if (variablesTableData.length > 0 && !initializationRef.current) {


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 variablesTableData is updated ? when adding or removing variables from the dashboard ?

vikrantgupta25 · 2024-12-17T12:21:35Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx

@@ -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);


why is onMount handling different ? onMount should be handled via the root triggers ?

vikrantgupta25 · 2024-12-17T12:22:10Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts

@@ -29,3 +29,110 @@ export const convertVariablesToDbFormat = (
 		result[id] = obj;
 		return result;
 	}, {});
+
+const getDependentVariables = (queryValue: string): string[] => {
+	const variableRegexPattern = /\{\{\s*?\.([^\s}]+)\s*?\}\}/g;


we do support a bunch of other variable formats as well, please do include them as well

vikrantgupta25 · 2024-12-17T12:23:46Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts

+	}
+
+	if (topologicalOrder.length !== Object.keys(dependencies).length) {
+		throw new Error('Cycle detected in the dependency graph!');


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 ?

vikrantgupta25 · 2024-12-17T12:29:02Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx

+									allSelected,
+									isMounted.current,
+								);
+								setVariablesToGetUpdated((prev) =>


onValueUpdate should be responsible to remove the variable from variablesToGetUpdated ?

github-actions · 2024-12-20T02:55:06Z

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

github-actions · 2024-12-24T06:15:25Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

❌ Changes requested. Incremental review on 5097375 in 32 seconds

More details

Looked at 518 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted 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.

ellipsis-dev · 2024-12-24T06:15:40Z

frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts

+export const buildDependencies = (
+	variables: IDashboardVariable[],
+): VariableGraph => {
+	console.log('buildDependencies', variables);


Remove the console.log statement to avoid unnecessary console output.

Suggested change

console.log('buildDependencies', variables);

srikanthccv · 2024-12-24T16:43:23Z

https://www.loom.com/share/a958edbb6f2f42b1881d60658fe8cbef

srikanthccv · 2024-12-24T16:54:31Z

@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?

SagarRajput-7 requested a review from YounixM as a code owner December 4, 2024 05:06

github-actions bot added docs required enhancement New feature or request labels Dec 4, 2024

ellipsis-dev bot reviewed Dec 4, 2024

View reviewed changes

frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts Show resolved Hide resolved

SagarRajput-7 added docs not required and removed docs required labels Dec 4, 2024

srikanthccv mentioned this pull request Dec 5, 2024

The monitoring data cannot be displayed on the host monitoring page and is displayed only after you refresh the page #6597

Open

vikrantgupta25 reviewed Dec 17, 2024

View reviewed changes

vikrantgupta25 requested changes Dec 17, 2024

View reviewed changes

vikrantgupta25 reviewed Dec 17, 2024

View reviewed changes

pranay01 deleted the branch main December 19, 2024 13:03

pranay01 closed this Dec 19, 2024

SagarRajput-7 reopened this Dec 20, 2024

SagarRajput-7 changed the base branch from develop to main December 20, 2024 02:54

SagarRajput-7 and others added 3 commits December 24, 2024 11:44

feat: updated the logic for variable update queue

645bb17

feat: added API limiting to reduce unnecessary api call for dashboard…

af840fb

… 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

SagarRajput-7 force-pushed the variable-update-queue branch from 1abf809 to 5097375 Compare December 24, 2024 06:14

SagarRajput-7 added the testing-deploy label Dec 24, 2024

SagarRajput-7 deployed to testing December 24, 2024 06:15 — with GitHub Actions Active

ellipsis-dev bot reviewed Dec 24, 2024

View reviewed changes

Merge branch 'main' into variable-update-queue

0c3282e

srikanthccv self-assigned this Dec 24, 2024

github-actions bot removed the testing-deploy label Dec 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: updated the logic for variable update queue #6586

feat: updated the logic for variable update queue #6586

SagarRajput-7 commented Dec 4, 2024 •

edited by ellipsis-dev bot

Loading

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

ellipsis-dev bot left a comment

github-actions bot commented Dec 4, 2024

cxposition commented Dec 5, 2024

SagarRajput-7 commented Dec 5, 2024

cxposition commented Dec 5, 2024

cxposition commented Dec 6, 2024

github-actions bot commented Dec 17, 2024

vikrantgupta25 Dec 17, 2024 •

edited

Loading

vikrantgupta25 Dec 17, 2024

vikrantgupta25 Dec 17, 2024

vikrantgupta25 Dec 17, 2024

vikrantgupta25 Dec 17, 2024

vikrantgupta25 Dec 17, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 24, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot Dec 24, 2024

srikanthccv commented Dec 24, 2024

srikanthccv commented Dec 24, 2024

feat: updated the logic for variable update queue #6586

Are you sure you want to change the base?

feat: updated the logic for variable update queue #6586

Conversation

SagarRajput-7 commented Dec 4, 2024 • edited by ellipsis-dev bot Loading

Summary

Related Issues / PR's

Screenshots

Comparison between local and prod env

Affected Areas and Manually Tested Areas

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024

cxposition commented Dec 5, 2024

SagarRajput-7 commented Dec 5, 2024

cxposition commented Dec 5, 2024

cxposition commented Dec 6, 2024

github-actions bot commented Dec 17, 2024

vikrantgupta25 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

vikrantgupta25 Dec 17, 2024

Choose a reason for hiding this comment

vikrantgupta25 Dec 17, 2024

Choose a reason for hiding this comment

vikrantgupta25 Dec 17, 2024

Choose a reason for hiding this comment

vikrantgupta25 Dec 17, 2024

Choose a reason for hiding this comment

vikrantgupta25 Dec 17, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 24, 2024

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Dec 24, 2024

Choose a reason for hiding this comment

srikanthccv commented Dec 24, 2024

srikanthccv commented Dec 24, 2024

SagarRajput-7 commented Dec 4, 2024 •

edited by ellipsis-dev bot

Loading

vikrantgupta25 Dec 17, 2024 •

edited

Loading