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

feat: enhance visualization of connectors #257

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

haoqixu
Copy link
Contributor

@haoqixu haoqixu commented Dec 11, 2023

partially implement #117

This PR enhances visualization of connectors:

  • The connection between connector's exporter and receiver is visualised as a directed arrows from exporter to receiver.
  • Pipeline Nodes (parentNode) are laid out based on the connector's connections. This is different from what was mentioned in the issue.

before:
图片

after:
图片

Copy link

vercel bot commented Dec 11, 2023

@haoqixu is attempting to deploy a commit to the OTelBin Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

github-actions bot commented Dec 11, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@haoqixu
Copy link
Contributor Author

haoqixu commented Dec 11, 2023

I have read the CLA Document and I hereby sign the CLA

d0etu added a commit to dash0hq/.github that referenced this pull request Dec 11, 2023
@mmanciop
Copy link
Member

Lovely :-) We'll review it ASAP!

Copy link

vercel bot commented Dec 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
otelbin ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2023 5:08pm

@mmanciop
Copy link
Member

@haoqixu would you please enable in your fork Allowing changes to a pull request branch created from a fork, or run (cd packages/otelbin/ && npm run prettier:all) from the repo root? The linter is unhappy because of missing semicolons :-)

@mmanciop
Copy link
Member

mmanciop commented Dec 12, 2023

Overall, I like very much where the change is going. The layout you implemented has a lot going for it: it is easier to see how pipelines flow into one another, following the conventional left-to-right approach.

I have concerns about the fact that it is no longer simple to see which pipelines can receive data "from the outside" through non-connector receivers, which exports send data to the outside, which is why in #117 I was thinking of always keeping lanes vertically aligned: receivers are always to the beginning of a "line", exporters always at its end.

Nevertheless, this looks to like an excellent improvement when dealing with connectors, and I am looking forward for @bripkens to review when he is no longer under the weather.

@mmanciop
Copy link
Member

I am also not sure how far it is realistic to go in terms of detecting connector cycles (see example), but we definitely should try not to have edges overlap with nodes.
Screenshot 2023-12-12 at 09 28 31

@haoqixu
Copy link
Contributor Author

haoqixu commented Dec 12, 2023

I am also not sure how far it is realistic to go in terms of detecting connector cycles (see example), but we definitely should try not to have edges overlap with nodes.

I think it is realistic to detect cycles and I have implemented a POC. It looks like:

图片 图片

After I clean up the code and test it more carefully, I will update the PR.

@haoqixu haoqixu force-pushed the feat-connect-connectors branch from f54ca2f to 9582a96 Compare December 13, 2023 10:44
@haoqixu
Copy link
Contributor Author

haoqixu commented Dec 13, 2023

I have rebased this PR onto the latest main and added the code for cycles detection. Here are the screenshots after update:
图片
图片

Copy link
Member

@bripkens bripkens left a comment

Choose a reason for hiding this comment

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

This is looking really good @haoqixu. Great job! ❤️

Left some minor comments about the code.

Could you please visually align the arrow head of the cycle indicator edge to the arrow heads of the other edge types? That would be great.

image

@haoqixu
Copy link
Contributor Author

haoqixu commented Dec 13, 2023

updated:
图片

Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
9.2% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@bripkens bripkens left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Really well done! Thank you @haoqixu!

@bripkens bripkens merged commit dea83eb into dash0hq:main Dec 13, 2023
7 of 8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@haoqixu haoqixu deleted the feat-connect-connectors branch December 14, 2023 06:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants