Skip to content
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

DAG View Migration to Plexus #1921

Closed

Conversation

prathamesh-mutkure
Copy link
Contributor

Which problem is this PR solving?

This is a draft PR for migrating the DAG view from Cytoscape to Plexus

Description of the changes

  • I've added two new tabs to System Architecture page, DAG_DirectedGraph and DAG_Diagraph
  • The DAG_DirectedGraph used Plexus DirectedGraph component for migration
  • The DAG_Diagraph used Plexus Digraph component for migration

Why migration using two components?

  • I managed to render the tree using DirectedGraph component but it didn't allow me to override the arrows/edges
  • The previous implementation used arrow to display callCount
  • Thus, I tried implementing the same graph using the Digraph component
  • The Diagraph migration is not done yet, thus the canvas is blank

I'll try my best to migrate the previous implementation, we'll go with the component that gives the best results

Screenshot 2023-10-31 at 3 14 25 PM

How was this change tested?

NA

Checklist

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (d429746) 96.54% compared to head (4e8b522) 95.90%.

❗ Current head 4e8b522 differs from pull request most recent head 6c32142. Consider uploading reports for the commit 6c32142 to get more accurate results

Files Patch % Lines
.../components/DependencyGraph/_DAG_DirectedGraph.tsx 3.33% 29 Missing ⚠️
...i/src/components/DependencyGraph/_DAG_Diagraph.tsx 4.16% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1921      +/-   ##
==========================================
- Coverage   96.54%   95.90%   -0.65%     
==========================================
  Files         256      258       +2     
  Lines        7604     7660      +56     
  Branches     1978     1987       +9     
==========================================
+ Hits         7341     7346       +5     
- Misses        263      314      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

@prathamesh-mutkure so reading through some of the comments, I think DirectedGraph is an older component and we probably should avoid using it. All other jaeger-ui sources only import Digraph, which seems newer. There's also this comment in the readme:

// TODO(joe): Update import after killing `DirectedGraph`
import Digraph from 'plexus/Digraph';

{...customProps}
/>

<text x={labelX - xOffset} y={labelY} fill="#000" fontSize="1rem" fontWeight="bold">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fontSize="1rem" fontWeight="bold"

is this something that can be expressed via stylesheet? How do we style the content of the nodes? Maybe label should also be a DOM content, not a plain text?

If too much work, ok to keep as plain text in the first iteration, but I'd still like to know the answer to the styling question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here is for the edge/arrow, nodes can be customised using renderNode property which is already being used for this graph, if you're talking about similar customization for edges then the documentation says it can be expensive

Screenshot 2023-11-14 at 3 33 00 PM

We can still try a renderEdge method and see how it affects the performance

markerStart={markerStart}
{...customProps}
/>
<g>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does <g> do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's there to group <path> and <text>, just for semantics, similar to <> ... </>

@@ -15,6 +15,8 @@
import * as React from 'react';

type TProps = Record<string, any> & {
id: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to change anything under deprecated DirectedGraph/..? Can your PR work without that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was for when I tried DirectedGraph, similar changes have been made to Diagraph, so we can skip these

yarn.lock Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

superseded by #1981

@yurishkuro yurishkuro closed this Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants