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(lineage) Update Lineage tab and Impact Analysis feature #5121

Conversation

chriscollins3456
Copy link
Collaborator

Makes a change to the Lineage tab so that we always show the Impact Analysis view but now we can toggle between Upstream and Downstream. We now have Filters default open and we are default selected "Downstream".

The default filter for degree of dependencies is going to be 1 to make load time quick on this tab. They can always change that filter. I've also set that filter at the top of the filters since it will be most important for this tab (I don't believe we show "Degree of Dependencies" elsewhere do we?

This meant doing a filter before query instead of after, so that we can be quicker about things. Therefore we now have a nullable param maxHops that we pass through the backend which will be 1 on load (instead of the default 1000 we have now).

The new look:
image

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Jun 8, 2022

Unit Test Results (build & test)

382 tests  ±0   382 ✔️ ±0   8m 59s ⏱️ -27s
  89 suites ±0       0 💤 ±0 
  89 files   ±0       0 ±0 

Results for commit 3ded253. ± Comparison against base commit e2f1da00.

♻️ This comment has been updated with latest results.

@@ -128,6 +128,7 @@ export default class EntityRegistry {
entity: relationship.entity as EntityInterface,
type: (relationship.entity as EntityInterface).type,
})),
numDownstreamChildren: genericEntityProperties?.downstream?.total,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this may actually cause a bug.. right now we show the number of upstream and downstream on an entity by getting the length of upstreamChildren and downstreamChildren - however these filter out some things to determine if the entity exists. Now if we just show total then we will probably start saying there are more upstream and downstream children than there actually show be.

Any suggestions on how to make this work in the UI? the point of using total is so that we don't have to query for the actual relationships and entities on entity page load, but just get the number.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes let's just show the total. I think that makes more sense than showing 100+

Comment on lines +30 to +31
upstreams={lineage?.numUpstreamChildren || 0}
downstreams={lineage?.numDownstreamChildren || 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@@ -90,12 +91,17 @@ export default function LineageEntityNode({
const [isExpanding, setIsExpanding] = useState(false);
const [expandHover, setExpandHover] = useState(false);
const { getAsyncEntity, asyncData } = useLazyGetEntityQuery();
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not just replace this line with const [getAsyncEntityLineage, { data: asyncLineageData }] = useGetEntityLineageLazyQuery();? Why do we need both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well we need both right now because the getAsyncLineage query only gets lineage data and we need this query to get all the other info about entities (like name, platform, etc.) - however I could update the lineage query to get everything we need and then we only have the one query.

Will do now

Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

Looks good although I was hoping this change would let us remove useLazyGetEntityQuery entirely... is that not the case?

loading: lineageLoading,
error: lineageError,
data: lineageData,
} = useGetEntityLineageQuery({ variables: { urn } });
const { loading, error, data } = useGetEntityQuery(urn, type);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here can we get rid of useGetEntityQuery?

Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

nice looks good!

@chriscollins3456 chriscollins3456 force-pushed the cc--public-update-lineage-tab branch from 33d5ad7 to 3ded253 Compare June 17, 2022 21:54
@@ -633,9 +635,22 @@ private void configureQueryResolvers(final RuntimeWiring.Builder builder) {
.dataFetcher("getRootGlossaryNodes", new GetRootGlossaryNodesResolver(this.entityClient))
.dataFetcher("entityExists", new EntityExistsResolver(this.entityService))
.dataFetcher("getNativeUserInviteToken", new GetNativeUserInviteTokenResolver(this.nativeUserService))
.dataFetcher("entity", getEntityResolver())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!!

import { useGetMlModelQuery } from '../../../graphql/mlModel.generated';
import { useGetMlModelGroupQuery } from '../../../graphql/mlModelGroup.generated';

export default function useGetEntityQuery(urn: string, entityType?: EntityType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh thank god

const [getAsyncMlModel, { data: asyncMlModel }] = useGetMlModelLazyQuery();
const [getAsyncMlModelGroup, { data: asyncMlModelGroup }] = useGetMlModelGroupLazyQuery();

const getAsyncEntity = useCallback(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh thank the lord

@@ -4,7 +4,7 @@ import { useEffect, useState } from 'react';
import { FacetMetadata } from '../../types.generated';
import { SearchFilter } from './SearchFilter';

const TOP_FILTERS = ['entity', 'tags', 'glossaryTerms', 'domains', 'owners'];
const TOP_FILTERS = ['degree', 'entity', 'tags', 'glossaryTerms', 'domains', 'owners'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

Awesome PR, @chriscollins3456 !

@jjoyce0510 jjoyce0510 merged commit 2841f32 into datahub-project:master Jun 21, 2022
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.

3 participants