fix(ingestion): add logging, make job more resilient to errors #4331

anshbansal · 2022-03-07T10:29:22Z

Added

Snowflake SaaS version check to know what version someone is running https://community.snowflake.com/s/topic/0TO0Z000000Unu5WAC/releases
Snowflake Role (useful in case a default role from username is being used)
Grants to the snowflake role (skipped for accountadmin)
DataHub CLI version in Source reports
DataHub GMS version in DataHub Rest Sink Reports

Fix

Missing permission from the snowflake documentation
Report error properly instead of huge stacktrace in case of snowflake credentials, looping any sql based tables/views
Warnings and errors not being added to reports for snowflake, bigquery source
Missing connect_args for lineage for some cases in snowflake sources

The gms version is shown in sink report as follows

{'records_written': 55,
 'warnings': [],
 'failures': [],
 'downstream_start_time': datetime.datetime(2022, 3, 7, 20, 7, 8, 207437),
 'downstream_end_time': datetime.datetime(2022, 3, 7, 20, 7, 49, 110622),
 'downstream_total_latency_in_seconds': 40.903185,
 'gms_version': 'RC'}

The added fields are shown in report for snowflake as follows

'cli_version': 'unavailable (installed in develop mode)',
'saas_version': '6.5.1',
 'role': 'datahub_role',
 'role_grants': ['USAGE granted on DATABASE ASEEM_TEST_DB',
                 'SELECT granted on EXTERNAL_TABLE ASEEM_TEST_DB.PUBLIC.ET1',
                 'SELECT granted on EXTERNAL_TABLE ASEEM_TEST_DB.PUBLIC.ET2',
                 'SELECT granted on EXTERNAL_TABLE ASEEM_TEST_DB.PUBLIC.TEST_BUCKET_ASEEM_TEST_CSV',
                 'USAGE granted on SCHEMA ASEEM_TEST_DB.PUBLIC',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.NORMAL_TABLE_FROM_EXTERNAL',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T1_V1_ET1',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T1_V1_ET2',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T1_V2_ET1',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T1_V2_ET2',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T2_V1_ET1',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T2_V1_ET2',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T2_V2_ET1',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.T2_V2_ET2',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.TABLE_EXTERNAL_TABLE_DOWNSTREAM',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.TABLE_VIEW_DOWNSTREAM',
                 'SELECT granted on TABLE ASEEM_TEST_DB.PUBLIC.TABLE_VIEW_DOWNSTREAM2',
                 'SELECT granted on VIEW ASEEM_TEST_DB.PUBLIC.NORMAL_VIEW_FROM_EXTERNAL_TABLE',
                 'SELECT granted on VIEW ASEEM_TEST_DB.PUBLIC.V1_ET1',
                 'SELECT granted on VIEW ASEEM_TEST_DB.PUBLIC.V1_ET2',
                 'SELECT granted on VIEW ASEEM_TEST_DB.PUBLIC.V2_ET1',
                 'SELECT granted on VIEW ASEEM_TEST_DB.PUBLIC.V2_ET2',
                 'OPERATE granted on WAREHOUSE COMPUTE_WH',
                 'USAGE granted on WAREHOUSE COMPUTE_WH']}

Failure report for wrong credentials snowflake

 'failures': {'version': ['Error: (snowflake.connector.errors.DatabaseError) 250001 (08001): Failed to connect to DB: '
                          'abcdef.snowflakecomputing.com:443. Incorrect username or password was specified.\n'
                          '(Background on this error at: http://sqlalche.me/e/13/4xp6)']},

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)

treff7es

Nice! :)

github-actions · 2022-03-07T11:26:37Z

Unit Test Results (metadata ingestion)

      5 files       5 suites 50m 36s ⏱️
  347 tests   347 ✔️   0 💤 0 ❌
1 579 runs 1 548 ✔️ 31 💤 0 ❌

Results for commit 6656e76.

♻️ This comment has been updated with latest results.

github-actions · 2022-03-07T12:07:45Z

Unit Test Results (build & test)

  39 files -   36   39 suites - 36 17s ⏱️ - 18m 1s
168 tests - 379 168 ✔️ - 326 0 💤 - 52 0 ❌ - 1

Results for commit 6656e76. ± Comparison against base commit 7efc88e.

♻️ This comment has been updated with latest results.

treff7es · 2022-03-07T14:52:16Z

metadata-ingestion/src/datahub/ingestion/source/sql/snowflake.py

            url,
            connect_args=self.config.get_sql_alchemy_connect_args(),
            **self.config.options,
        )

+    def inspect_version(self) -> Any:
+        db_engine = self.get_metadata_engine()


Is it possible this will be null?

I don't think so. Do you see any case?

treff7es · 2022-03-07T14:53:24Z

metadata-ingestion/src/datahub/ingestion/source/sql/snowflake.py

+
+    def inspect_role_grants(self) -> Any:
+        db_engine = self.get_metadata_engine()
+        cur_role = None


You can make this equal to self.config.role

Did not want to edit the existing config so used a different variable. I am putting it in self.report.role for reporting.

jjoyce0510 · 2022-03-07T20:43:08Z

metadata-ingestion/source_docs/snowflake.md

@@ -27,6 +27,9 @@ grant select on all views in database <your-database> to role datahub_role;
 grant usage on future schemas in database "<your-database>" to role datahub_role;
 grant select on future tables in database "<your-database>" to role datahub_role;

+// Grant privileges on snowflake default database - needed for lineage
+grant imported privileges on DATABASE snowflake to role datahub_role;


This is already on line 46

jjoyce0510 · 2022-03-07T20:43:08Z

metadata-ingestion/source_docs/snowflake.md

@@ -27,6 +27,9 @@ grant select on all views in database <your-database> to role datahub_role;
 grant usage on future schemas in database "<your-database>" to role datahub_role;
 grant select on future tables in database "<your-database>" to role datahub_role;

+// Grant privileges on snowflake default database - needed for lineage
+grant imported privileges on DATABASE snowflake to role datahub_role;


This is already on line 46

Handled in #4341. Thanks for catching

…ub-project#4331)

feat(snowflake): add role, sass version, and role grants check

ec4ad16

treff7es approved these changes Mar 7, 2022

View reviewed changes

anshbansal changed the title ~~feat(snowflake): add role, sass version, and role grants check~~ feat(snowflake): add role, saas version, and role grants check Mar 7, 2022

fix linting

785775d

Merge remote-tracking branch 'origin' into check_snowflake_grants

aaf7225

fail fast credentials, don't fail job in case of a schema problem

fdb5586

anshbansal changed the title ~~feat(snowflake): add role, saas version, and role grants check~~ feat(snowflake): add logging, make job more resilient to errors Mar 7, 2022

anshbansal changed the title ~~feat(snowflake): add logging, make job more resilient to errors~~ feat(ingestion): add logging, make job more resilient to errors Mar 7, 2022

anshbansal added 2 commits March 7, 2022 18:04

fix linting

845206d

fix warning being sent properly, fix docs missing privileges

2350e1a

anshbansal changed the title ~~feat(ingestion): add logging, make job more resilient to errors~~ fix(ingestion): add logging, make job more resilient to errors Mar 7, 2022

anshbansal added 4 commits March 7, 2022 19:11

fix linting

b9ab3dc

add cli version in base report

3b067c7

add gms version in rest sink

dbbeeac

fix: add missed errors in error reports

0afc323

treff7es reviewed Mar 7, 2022

View reviewed changes

anshbansal added 2 commits March 7, 2022 20:39

fix error/warn methods

d7767e5

add logging for snowflake usage

6656e76

jjoyce0510 reviewed Mar 7, 2022

View reviewed changes

jjoyce0510 approved these changes Mar 7, 2022

View reviewed changes

shirshanka approved these changes Mar 7, 2022

View reviewed changes

shirshanka merged commit beb51eb into datahub-project:master Mar 7, 2022

anshbansal deleted the check_snowflake_grants branch March 8, 2022 05:15

anshbansal mentioned this pull request Mar 8, 2022

fix(doc): remove duplicate entry for permission #4341

Merged

4 tasks

maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022

fix(ingestion): add logging, make job more resilient to errors (datah…

dc15851

…ub-project#4331)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ingestion): add logging, make job more resilient to errors #4331

fix(ingestion): add logging, make job more resilient to errors #4331

anshbansal commented Mar 7, 2022 •

edited

Loading

treff7es left a comment

github-actions bot commented Mar 7, 2022 •

edited

Loading

github-actions bot commented Mar 7, 2022 •

edited

Loading

treff7es Mar 7, 2022

anshbansal Mar 7, 2022

treff7es Mar 7, 2022

anshbansal Mar 7, 2022

jjoyce0510 Mar 7, 2022

jjoyce0510 Mar 7, 2022

anshbansal Mar 8, 2022

fix(ingestion): add logging, make job more resilient to errors #4331

fix(ingestion): add logging, make job more resilient to errors #4331

Conversation

anshbansal commented Mar 7, 2022 • edited Loading

Checklist

treff7es left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2022 • edited Loading

Unit Test Results (metadata ingestion)

github-actions bot commented Mar 7, 2022 • edited Loading

Unit Test Results (build & test)

treff7es Mar 7, 2022

Choose a reason for hiding this comment

anshbansal Mar 7, 2022

Choose a reason for hiding this comment

treff7es Mar 7, 2022

Choose a reason for hiding this comment

anshbansal Mar 7, 2022

Choose a reason for hiding this comment

jjoyce0510 Mar 7, 2022

Choose a reason for hiding this comment

jjoyce0510 Mar 7, 2022

Choose a reason for hiding this comment

anshbansal Mar 8, 2022

Choose a reason for hiding this comment

anshbansal commented Mar 7, 2022 •

edited

Loading

github-actions bot commented Mar 7, 2022 •

edited

Loading

github-actions bot commented Mar 7, 2022 •

edited

Loading