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.

Sign up for GitHub

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

Jump to bottom

fix(ingestion/metabase): Fetch Dashboards through Collections #9631

Merged
merged 19 commits into from
Jan 26, 2024
Merged

fix(ingestion/metabase): Fetch Dashboards through Collections #9631

merged 19 commits into from
Jan 26, 2024

Conversation

7onn
Copy link
Contributor

@7onn 7onn commented Jan 15, 2024 •
edited by hsheth2
Loading

The endpoint /api/dashboard was removed, which caused DataHub Metabase ingestion to break. Then, Metabase re-added it to fix compatibility issues. But they still are going to remove it in the next release. This PR updates the Metabase's Ingestion to follow the Metabase's recommended method:

No guarantees will be made to keep it up-to-date with the officially supported alternative, which is to use GET /api/collection to get the list of collections, and for each collection use GET /api/collection/:id/items?models=dashboard, and finally GET /api/dashboard/:id to get a specific dashboard.

Related issues:

Notes

I didn't succeed on running the integration tests. Maybe the CI can run them for me if approved.

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 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 15, 2024
@7onn 7onn marked this pull request as ready for review January 15, 2024 15:58
@7onn 7onn marked this pull request as draft January 15, 2024 16:53
@7onn 7onn changed the title fix(ingestion/metabase): Update dashboard URL feat(ingestion/metabase): Fetch Dashboards through Collections Jan 15, 2024
@7onn 7onn marked this pull request as ready for review January 15, 2024 21:08
@7onn 7onn changed the title feat(ingestion/metabase): Fetch Dashboards through Collections fix(ingestion/metabase): Fetch Dashboards through Collections Jan 16, 2024
@7onn 7onn marked this pull request as draft January 16, 2024 13:02
@7onn 7onn marked this pull request as ready for review January 16, 2024 13:38
hsheth2
hsheth2 reviewed Jan 18, 2024
View reviewed changes
Copy link
Collaborator

@hsheth2 hsheth2 left a comment

Choose a reason for hiding this comment

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

The code mostly looks good, just had a few minor comments

metadata-ingestion/tests/integration/metabase/test_metabase.py
@@ -14,7 +14,8 @@
JSON_RESPONSE_MAP = {
"http://localhost:3000/api/session": "session.json",
"http://localhost:3000/api/user/current": "user.json",
"http://localhost:3000/api/dashboard": "dashboard.json",
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we delete the dashboard.json file? or is that still used elsewhere in the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I struggled to run these tests, actually. I did set my machine to java 8 but couldn't run the gradlew successfully.
I'll put some more thought to it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hsheth2 I'm using my GH Actions to test it https://github.com/7onn/datahub/actions
But I couldn't find how to run these python tests nor run the gradlew from this mac.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We've actually migrated to java 17 now

Have you taken a look at these docs https://datahubproject.io/docs/next/metadata-ingestion/developing/

Let me know if you run into issues with it - we want developing on ingestion, including tests, to be a smooth experience

metadata-ingestion/src/datahub/ingestion/source/metabase.py Outdated Show resolved Hide resolved
metadata-ingestion/src/datahub/ingestion/source/metabase.py Outdated Show resolved Hide resolved
metadata-ingestion/src/datahub/ingestion/source/metabase.py Outdated Show resolved Hide resolved
hsheth2
hsheth2 reviewed Jan 22, 2024
View reviewed changes
.github/workflows/build-and-test.yml Outdated
@@ -3,6 +3,7 @@ on:
push:
branches:
- master
- 7onn/update-metabase-dashboard-url
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- 7onn/update-metabase-dashboard-url

Copy link
Collaborator

Choose a reason for hiding this comment

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

mainly just need to do this before merging the PR in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done by: d9022bc

metadata-ingestion/tests/integration/metabase/test_metabase.py
@@ -14,7 +14,8 @@
JSON_RESPONSE_MAP = {
"http://localhost:3000/api/session": "session.json",
"http://localhost:3000/api/user/current": "user.json",
"http://localhost:3000/api/dashboard": "dashboard.json",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We've actually migrated to java 17 now

Have you taken a look at these docs https://datahubproject.io/docs/next/metadata-ingestion/developing/

Let me know if you run into issues with it - we want developing on ingestion, including tests, to be a smooth experience

@7onn 7onn requested a review from hsheth2 January 24, 2024 17:43
hsheth2
hsheth2 reviewed Jan 24, 2024
View reviewed changes
metadata-ingestion/tests/integration/metabase/metabase_mces_golden.json Outdated
"charts": [
"urn:li:chart:(metabase,1)",
"urn:li:chart:(metabase,2)"
"charts": [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like the relationships between dashboards and charts disappeared? Given that it's an important part of the integration, can we update the code / test data to make sure we're testing the dashboard -> chart relationship generation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by:

@7onn 7onn requested a review from hsheth2 January 25, 2024 11:19
@hsheth2 hsheth2 added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Jan 25, 2024
@hsheth2 hsheth2 merged commit f7f0b14 into datahub-project:master Jan 26, 2024
53 checks passed
@7onn 7onn deleted the 7onn/update-metabase-dashboard-url branch January 26, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsheth2 hsheth2 hsheth2 approved these changes

Assignees

@hsheth2 hsheth2

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metabase ingestion is using an old dashboard URL
2 participants
@7onn @hsheth2