feat(ingestion/tableau): optionally ingest multiple sites and create site containers #10498

haeniya · 2024-05-14T09:32:42Z

Description

With this PR, the Tableau ingestion can be configured to ingest multiple sites at once and to add the sites as containers. We have around 26 sites in the company, and it would be pretty cumbersome to manage 26 ingestions separately. Additionally, because we have so many sites and projects, we need the sites to show up as containers in Datahub.

To achieve this I introduced some new optional config properties: ingest_multiple_sites, add_site_container and site_name_pattern. All existing recipes should continue to work as-is.

A recipe could look something like this:

source:
  type: tableau
  config:
    # Coordinates
    connect_uri: https://tableau1-test.swisscom.com
    ingest_multiple_sites: True
    add_site_container: True
    site_name_pattern:
      allow: ["^Data.*$"]
    platform_instance: tableau_test

    # Credentials
    username: "${TABLEAU_USER}"
    password: "${TABLEAU_PASSWORD}"

sink:
  type: "datahub-rest"
  config:
    server: "http://localhost:8080"

And that's how it looks like after ingesting:

I already saw this asked a few times in the Slack channel as well:

Please let me know what you think and I will proceed with adding some documentation and tests to this.

Many thanks.

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

…e containers

…e method instead of manually switching site

haeniya · 2024-05-14T09:35:11Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

@@ -762,8 +809,9 @@ def _populate_projects_registry(self) -> None:

    def _authenticate(self) -> None:
        try:
-            self.server = self.config.make_tableau_client()
-            logger.info("Authenticated to Tableau server")
+            site_content_url = self.current_site.content_url if self.current_site else self.config.site


This is done because I saw there is some code that does re-authentication:

datahub/metadata-ingestion/src/datahub/ingestion/source/tableau.py

Line 814 in 3dc5326

self._authenticate()

With this we always re-authenticate with the current site or fallback to the site property provided in the config (when authenticating the first time there's no current_site set).

haeniya · 2024-05-14T09:37:25Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

+
+        # We rely on automatic browse paths (v2) when creating containers. That's why we need to sort the projects here.
+        # Otherwise, nested projects will not have the correct browse paths if not created in correct order / hierarchy.
+        self.tableau_project_registry = OrderedDict(sorted(projects_to_ingest.items(), key=lambda item: len(item[1].path)))


This is related to a bug we were experiencing when dealing with nested projects. If nested projects were added before their parent the browse path (v2) would be incorrect (wrong hierarchy). Let me know if there is a better way to do this.

This works great, and is a good catch

haeniya · 2024-05-14T10:03:07Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

+                    if site.state != 'Active' or not self.config.site_name_pattern.allowed(site.name):
+                        continue
+                    self.current_site = site
+                    self.server.auth.switch_site(site)


I just noticed, that the documentation states that this method is not allowed with Tableau Cloud: https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_ref_authentication.htm#switch_site
Unfortunately, I'm not able to test it with Tableau Cloud. I had a solution in a previous commit that did the switch manually (by just re-authenticating). We could go back to this if needed. WDYT? Do we need to support Tableau Cloud?

Yes we do need support for tableau cloud, so it would be better to manually switch

One other thing I need to test on my end - I want to double check that listing sites is supported with tableau cloud

haeniya · 2024-05-21T06:19:59Z

I tried to fix the tests but for some reason when I run pytest tests/integration/tableau/test_tableau_ingest.py --update-golden-files most of the golden files get changed to an empty list which I don't think is correct. Btw - I already started to mock the new Tableau server methods in tableau_ingest_common. Any idea what I'm doing wrong?

hsheth2

Had some comments about code structuring, and we'll probably need a test for the ingest_multiple_sites and add_site_container functionality

hsheth2 · 2024-05-29T03:50:35Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

+    add_site_container: bool = Field(
+        False,
+        description="When enabled, sites are added as containers and therefore visible in the folder structure within Datahub.",
+    )


eventually it will make sense to make ingest_multiple_sites and add_site_container default to enabled, right

hsheth2 · 2024-05-29T03:51:58Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

@@ -536,6 +560,7 @@ def __init__(
        self.tableau_project_registry: Dict[str, TableauProject] = {}
        self.workbook_project_map: Dict[str, str] = {}
        self.datasource_project_map: Dict[str, str] = {}
+        self.current_site: Optional[SiteItem] = None


can we call _init_ingestion_variables here

I'd prefer to have all variable initialization in a single spot

broadly, resetting initialization variables is usually a sign that the class isn't specific enough. I think it makes sense to have a TableauSiteSource class that is site-specific, and the TableauSource simply lists the sites and then calls TableauSiteSource.get_workunits_internal()

Most of the existing methods would move to the TableauSiteSource, so this would be a pretty small refactoring. The state management would also become a lot easier to follow and structurally guarantee that we don't forget to reset some variable, and would mean that we need fewer tests for this code

hsheth2 · 2024-05-29T03:53:23Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

+                    if (
+                        site.state != "Active"
+                        or not self.config.site_name_pattern.allowed(site.name)
+                    ):


would be good to log these as skipped

hsheth2 · 2024-05-29T03:55:32Z

metadata-ingestion/src/datahub/ingestion/source/tableau.py

+                    if site.state != 'Active' or not self.config.site_name_pattern.allowed(site.name):
+                        continue
+                    self.current_site = site
+                    self.server.auth.switch_site(site)


Yes we do need support for tableau cloud, so it would be better to manually switch

One other thing I need to test on my end - I want to double check that listing sites is supported with tableau cloud

Yanik Häni added 3 commits May 14, 2024 07:54

feat(ingest/tableau): optionally ingest multiple sites and create sit…

ec6aef3

…e containers

fix(ingestion/tableau): fix wrong site id when fetching site

20a1d4e

refactor(ingestion/tableau): refactor code to use built-in switch_sit…

acc4480

…e method instead of manually switching site

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 May 14, 2024

haeniya commented May 14, 2024

View reviewed changes

vercel bot had a problem deploying to Preview May 14, 2024 09:41 Failure

haeniya commented May 14, 2024

View reviewed changes

fix(linting): fix linting issues and cleanup

31c8f18

vercel bot had a problem deploying to Preview May 14, 2024 13:38 Failure

hsheth2 reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingestion/tableau): optionally ingest multiple sites and create site containers #10498

feat(ingestion/tableau): optionally ingest multiple sites and create site containers #10498

haeniya commented May 14, 2024 •

edited

haeniya May 14, 2024 •

edited

haeniya May 14, 2024 •

edited

hsheth2 May 29, 2024

haeniya May 14, 2024 •

edited

hsheth2 May 29, 2024

haeniya commented May 21, 2024 •

edited

hsheth2 left a comment

hsheth2 May 29, 2024

hsheth2 May 29, 2024

hsheth2 May 29, 2024

hsheth2 May 29, 2024

hsheth2 May 29, 2024

feat(ingestion/tableau): optionally ingest multiple sites and create site containers #10498

Are you sure you want to change the base?

feat(ingestion/tableau): optionally ingest multiple sites and create site containers #10498

Conversation

haeniya commented May 14, 2024 • edited

Description

Checklist

haeniya May 14, 2024 • edited

Choose a reason for hiding this comment

haeniya May 14, 2024 • edited

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

haeniya May 14, 2024 • edited

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

haeniya commented May 21, 2024 • edited

hsheth2 left a comment

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

hsheth2 May 29, 2024

Choose a reason for hiding this comment

haeniya commented May 14, 2024 •

edited

haeniya May 14, 2024 •

edited

haeniya May 14, 2024 •

edited

haeniya May 14, 2024 •

edited

haeniya commented May 21, 2024 •

edited