Skip to content

feat(ingest/superset): add domains and platform_instance support - #9647

Merged
hsheth2 merged 14 commits into
datahub-project:masterfrom
rinzool:feat/add-domain-argument-for-superset
Jan 21, 2024
Merged

hsheth2 merged 14 commits into
datahub-project:masterfrom
rinzool:feat/add-domain-argument-for-superset

Conversation

@rinzool

@rinzool rinzool commented Jan 17, 2024 •

Copy link
Copy Markdown
Contributor

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

Description

Hi, in my company we really need a simple way to add a domain to chart/dashboard entities coming from Superset.
So I copied what is done for Glue ingestion and add a domain argument in the config

I tried locally and it works as I expected 👌

Furthermore I add a platform_instance argument and deprecate env argument that commit b088512 (It may be done in another PR if necessary)

Example output when running ingestion:

Config:

source:
  type: superset
  config:
    # Coordinates
    connect_uri: ***
    env: "STG"
    platform_instance: "my_awesome_platform"
    domain: 
      my-domain:
        allow: [".*"]

    # Credentials
    username: admin
    password: ***
    provider: db

Output:

ConfigurationWarning: env is deprecated and will be removed in a future release. Please use platform_instance instead.
...
{'events_produced': 455,
 'events_produced_per_sec': 44,
 'entities': {'dashboard': ['urn:li:dashboard:(superset,my_awesome_platform.623)',
                            'urn:li:dashboard:(superset,my_awesome_platform.433)',
                            'urn:li:dashboard:(superset,my_awesome_platform.171)',
                            'urn:li:dashboard:(superset,my_awesome_platform.476)',
                            'urn:li:dashboard:(superset,my_awesome_platform.385)',
                            'urn:li:dashboard:(superset,my_awesome_platform.239)',
                            'urn:li:dashboard:(superset,my_awesome_platform.219)',
                            'urn:li:dashboard:(superset,my_awesome_platform.188)',
                            'urn:li:dashboard:(superset,my_awesome_platform.155)',
                            'urn:li:dashboard:(superset,my_awesome_platform.19)',

@github-actions github-actions Bot added ingestion PR or Issue related to the ingestion of metadata community-contribution PR or Issue raised by member(s) of DataHub Community labels Jan 17, 2024
@rinzool rinzool changed the title feat: add a new argument domani to superset source feat(superset): add a new argument domani to superset source Jan 17, 2024
@rinzool rinzool changed the title feat(superset): add a new argument domani to superset source feat(superset): add a new argument domain and platform_instance to superset source Jan 18, 2024

def construct_dashboard_from_api_data(self, dashboard_data):
dashboard_urn = f"urn:li:dashboard:({self.platform},{dashboard_data['id']})"
dashboard_urn = f"urn:li:dashboard:({self.platform},{self.platform_instance}{dashboard_data['id']})"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's use our make_chart_urn and make_dashboard_urn helpers - they also have a proper platform_instance kwargs, which should also let us avoid needing self.platform_instance

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the tip 👍
It works fine perfectly 👌

Example of logs:

 'entities': {'dashboard': ['urn:li:dashboard:(superset,prod-bedrock.508)',
                            'urn:li:dashboard:(superset,prod-bedrock.543)',
                            'urn:li:dashboard:(superset,prod-bedrock.242)',
                            'urn:li:dashboard:(superset,prod-bedrock.337)',
                            'urn:li:dashboard:(superset,prod-bedrock.509)',
                            'urn:li:dashboard:(superset,prod-bedrock.567)',
                            'urn:li:dashboard:(superset,prod-bedrock.118)',
                            'urn:li:dashboard:(superset,prod-bedrock.428)',
                            'urn:li:dashboard:(superset,prod-bedrock.340)',
                            'urn:li:dashboard:(superset,prod-bedrock.183)',
                            '... sampled of 212 total elements'],
              'chart': ['urn:li:chart:(superset,prod-bedrock.3143)',
                        'urn:li:chart:(superset,prod-bedrock.9784)',
                        'urn:li:chart:(superset,prod-bedrock.11400)',
                        'urn:li:chart:(superset,prod-bedrock.11399)',
                        'urn:li:chart:(superset,prod-bedrock.11398)',
                        'urn:li:chart:(superset,prod-bedrock.10060)',
                        'urn:li:chart:(superset,prod-bedrock.10052)',
                        'urn:li:chart:(superset,prod-bedrock.10050)',
                        'urn:li:chart:(superset,prod-bedrock.10046)',
                        'urn:li:chart:(superset,prod-bedrock.11396)',

def get_report(self) -> StaleEntityRemovalSourceReport:
return self.report

def _gen_domain_urn(self, title: str) -> Optional[str]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we have a "DomainRegistry" helper, which lets us convert domain names into urns using a lookup against DataHub - let's see if we can reuse the same pattern

A good example to copy would be the kafka integration's domain support:

for domain, pattern in self.source_config.domain.items():

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks, it's done 👍 Let me know if it is what you expected

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Although, I removed the _get_domain_urn to have only one function _get_domain_wu (I don't think it's relevant to have two distinct functions here)

@rinzool
rinzool force-pushed the feat/add-domain-argument-for-superset branch 2 times, most recently from 480a18e to bd91e40 Compare January 19, 2024 08:47

@hsheth2 hsheth2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@hsheth2 hsheth2 added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Jan 20, 2024
@anshbansal

Copy link
Copy Markdown
Collaborator

@rinzool We merged in your other superset PRs. Seems they caused a conflict. If you could resolve that and ensure lint is passing we can get this merged in

@rinzool
rinzool force-pushed the feat/add-domain-argument-for-superset branch 2 times, most recently from 7b4a3f9 to baa0565 Compare January 20, 2024 13:02
@rinzool

rinzool commented Jan 20, 2024 •

Copy link
Copy Markdown
Contributor Author

@rinzool We merged in your other superset PRs. Seems they caused a conflict. If you could resolve that and ensure lint is passing we can get this merged in

Yep with 3 PRs I expected some conflicts 😅 I resolved them, CI should be successful (it's ok locally)

Note that I can edit unit tests to add a test for domain and platform instance too

@rinzool
rinzool force-pushed the feat/add-domain-argument-for-superset branch from baa0565 to 0d8fd42 Compare January 20, 2024 16:37
@hsheth2 hsheth2 changed the title feat(superset): add a new argument domain and platform_instance to superset source feat(ingest/superset): add domains and platform_instance support Jan 21, 2024
@hsheth2
hsheth2 merged commit 344eeae into datahub-project:master Jan 21, 2024
@rinzool
rinzool deleted the feat/add-domain-argument-for-superset branch January 22, 2024 08:42
david-leifker pushed a commit that referenced this pull request May 27, 2026
… mappings (#9647)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Danishlynx added a commit to Danishlynx/datahub that referenced this pull request Sep 23, 2026
…harts

Superset has accepted platform_instance since datahub-project#9647 but never emitted
the dataPlatformInstance aspect, so the Navigate panel files every
dashboard and chart under "Default" no matter how many deployments are
ingested.

Add a get_data_platform_instance helper, modelled on Tableau's except
that it returns nothing when platform_instance is unset, so existing
deployments and the five existing goldens are untouched. Append the
aspect at the dashboard and chart snapshot sites and declare the
PLATFORM_INSTANCE capability. Preset builds on this source and inherits
both with no code of its own; a unit test pins that its aspect resolves
to the preset platform.

Datasets are deliberately left out. Their URNs are built through
get_datasource_urn_from_id with plain make_dataset_urn and carry no
instance (the instance-aware gen_dataset_urn helper exists but is
unused), so an aspect on them would name an instance the URN does not
have, and two deployments exposing the same table would overwrite each
other's aspect on the one entity. Making the dataset URN instance-aware
is a breaking change that also moves chart-to-dataset lineage, so it
belongs with the containers follow-up. A unit test pins the omission and
the TODO says why.

Tests: unit present/absent cases for dashboards and charts, an
empty-string case, the Preset inheritance cases, and one integration
test with a new golden that sets platform_instance. Docs: a Platform
instance section on both connector pages. The connector registry JSON
is regenerated.

Fixes datahub-project#11153

This branch was successfully deployed

1 active deployment
Preview — 0d8fd42b Deployed Jan 20, 2024 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata merge-pending-ci A PR that has passed review and should be merged once CI is green.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants