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 12 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.
Check warning on line 72 in metadata-ingestion/src/datahub/ingestion/source/grafana/field_utils.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/field_utils.py#L72
Check warning on line 74 in metadata-ingestion/src/datahub/ingestion/source/grafana/field_utils.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/field_utils.py#L74
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 160 in metadata-ingestion/src/datahub/ingestion/source/grafana/field_utils.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/field_utils.py#L158-L160
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.
Check warning on line 70 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L67-L70
Check warning on line 75 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L75
Check warning on line 86 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L85-L86
Check warning on line 91 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L91
Check warning on line 122 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L121-L122
Check warning on line 127 in metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py
Codecov / codecov/patch
metadata-ingestion/src/datahub/ingestion/source/grafana/grafana_api.py#L127
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.
These two may be removed if using
PlatformInstanceConfigMixin
andEnvConfigMixin
inGrafanaSourceConfig
, are they really needed atPlatformConnectionConfig
?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.
Updated the to use the parent class
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.
platform_instance
andenv
still there as well asplatform_instance
inGrafanaSourceConfig
🤔what parent class do you refer to?