amundsen-io / amundsen

Amundsen is a metadata driven application for improving the productivity of data analysts, data scientists and engineers when interacting with data.
https://www.amundsen.io/amundsen/
Apache License 2.0
4.43k stars 961 forks source link

feat: Change default table lineage depth to 1 #2069

Closed jsnb-devoted closed 1 year ago

jsnb-devoted commented 1 year ago

Summary of Changes

Changing the default lineage depth from 5 to 1. Not sure if this is appealing to anyone but I thought I would put this out there. Typically people don't need to see the lineage at this broad a depth. Ideally this parameter would be selectable in the UI but until then defaulting it to 1 makes the page more usable for me. If other folks disagree we can just close this.

Tests

Documentation

CheckList

Make sure you have checked all steps below to ensure a timely review.

boring-cyborg[bot] commented 1 year ago

Congratulations on your first Pull Request and welcome to Amundsen community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/amundsen-io/amundsen/blob/main/CONTRIBUTING.md)

Golodhros commented 1 year ago

Hi @jsnb-devoted, I chatted with the team, and it seems there is a need here to expand our configuration object to include the default depth of the graph as you suggest.

The changes would be:

I know is it a bit more work, would you be OK to do this?

jsnb-devoted commented 1 year ago

I know is it a bit more work, would you be OK to do this?

hi @Golodhros - That seems straightforward enough. Thanks for outlining all the places this would need to change.

Sounds like we want the depth to be configurable regardless but another thing that I doubt I have the bandwidth or front end experience for is defaulting all of the nodes one level of depth away from the root node to be collapsed. That way the user loads the page and it appears as if the graph is only one level of depth but they can click the nodes to expand them. I took a look at the code for the graph and it seemed a bit more advanced than this config change.

Golodhros commented 1 year ago

I know is it a bit more work, would you be OK to do this?

hi @Golodhros - That seems straightforward enough. Thanks for outlining all the places this would need to change.

Sounds like we want the depth to be configurable regardless but another thing that I doubt I have the bandwidth or front end experience for is defaulting all of the nodes one level of depth away from the root node to be collapsed. That way the user loads the page and it appears as if the graph is only one level of depth but they can click the nodes to expand them. I took a look at the code for the graph and it seemed a bit more advanced than this config change.

I see. Sadly I won't be able to help on this, as I am not super familiar with the graph code and we don't use it at Lyft, so my priorities are not aligned with improving that code.

Maybe you could do a git blame and reach out the authors?

jsnb-devoted commented 1 year ago

I know is it a bit more work, would you be OK to do this?

hi @Golodhros - That seems straightforward enough. Thanks for outlining all the places this would need to change. Sounds like we want the depth to be configurable regardless but another thing that I doubt I have the bandwidth or front end experience for is defaulting all of the nodes one level of depth away from the root node to be collapsed. That way the user loads the page and it appears as if the graph is only one level of depth but they can click the nodes to expand them. I took a look at the code for the graph and it seemed a bit more advanced than this config change.

I see. Sadly I won't be able to help on this, as I am not super familiar with the graph code and we don't use it at Lyft, so my priorities are not aligned with improving that code.

Maybe you could do a git blame and reach out the authors?

@ozandogrultan or @bbsbb any interest in this feature? Or maybe can describe the path to implementing? The tl;dr is default all of the level 1 nodes in the table lineage view to be collapsed instead of expanded.

Golodhros commented 1 year ago

You only need to sign the commits so we can merge it.

Thanks a lot!

jsnb-devoted commented 1 year ago

I'm botching signing the old commits and I just accidentally pulled in another commit. I'm figuring this out.

jsnb-devoted commented 1 year ago

Fixed in a follow up PR https://github.com/amundsen-io/amundsen/pull/2075