Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(ingestion/grafana): Add datasets and charts to dashboards with lineage and tags. Lineage back to source #12417
base: master
Are you sure you want to change the base?
feat(ingestion/grafana): Add datasets and charts to dashboards with lineage and tags. Lineage back to source #12417
Changes from all commits
d4be2de
660daf8
6cfbf5f
23e4eca
5468909
e684eed
eda52c4
507c811
0e9f0dc
c832e91
0fe68d2
c0e63d7
64a3bae
09b1ed8
b06fd12
e0533ad
27a9ceb
a7e9e19
4714b24
f0f88c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this in the codebase — does it fit the purpose?
datahub/metadata-ingestion/src/datahub/utilities/dedup_list.py
Line 6 in 8eda51e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gave it a try and unfortunately it doesn't do the business. I gave it a go and it was causing hashing issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that split by
,
will actually split by columns may be wrong assumption,eg
:SELECT CONCAT(first_name, ' ', last_name) AS full_name FROM users
Not sure if we need to address this complexity though. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we log some warning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put this in logger but were you looking for this to be in the report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger should be fine
given we are catching three types of exceptions, adding the exception to the log trace may help to discriminate the issue
Check warning on line 45 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L44-L45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the experience with other sources, it's nice to have this as a config parameter with a default value (
page_size
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to just specify the page size to reduce the overhead of anyone needing to think about changing this, but happy to change.