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.

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

🎉Categorized Config Errors Accurately for Google Analytics 4 (GA4) and Google Ads #25987

Conversation

lazebnyi
Copy link
Collaborator

@lazebnyi lazebnyi commented May 11, 2023

What

Google Analytics 4 (GA4)

The errors encountered with GAv4 should be classified as configuration errors rather than system errors.

Error 403: Client Error - Forbidden for URL: https://analyticsdata.googleapis.com/v1beta/properties/164523383/metadata
Error 400: Client Error - Bad Request for URL: https://analyticsdata.googleapis.com/v1beta/properties/UA-164523383-1/metadata
Error 400: Client Error - Bad Request for URL: https://analyticsdata.googleapis.com/v1beta/properties/a164523383w230249301p216691667/metadata

The error message should be revised to: "Access was denied to the entered Property ID. Please verify your access to the Property ID or refer to Google Analytics' documentation to find your Property ID."

Further enhancements to the specification:
The tooltip should display an example value.
The Property ID field should only accept numeric values and disallow entries such as "UA-165496".

Google Ads

The classification of these errors should be changed from system errors to configuration errors:

Error: Incorrect GAQL query statement: '"SELECT campaign.id, campaign.name FROM campaign"'
Error: Incorrect GAQL query statement: 'customer_query_2'
Error: Incorrect GAQL query statement: 'keyword_stats'

How

With try..except catch error and raise AirbyteTracedException with config_error failure type.

@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan and you've followed all steps in the Breaking Changes Checklist
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • You, or an Airbyter, have run /test successfully on this PR - or on a non-forked branch
  • You, or an Airbyter, have run /publish successfully on this PR - or on a non-forked branch
  • You've updated the connector's metadata.yaml file (new!)
  • The Octavia bot updated the source_definitions.yaml or destination_definitions.yaml, or you ran processResources manually (deprecated)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented May 11, 2023

/test connector=connectors/source-google-analytics-data-api

🕑 connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/4947619403
❌ connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/4947619403
🐛 https://gradle.com/s/3hkktlefeqdii

Build Failed

Test summary info:

Could not find result summary

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented May 11, 2023

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/4947622532
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/4947622532
Python tests coverage:

Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/utils.py                    61      1    98%
source_google_ads/streams.py                 195      6    97%
source_google_ads/source.py                   95      7    93%
source_google_ads/custom_query_stream.py      56      5    91%
source_google_ads/google_ads.py               73     12    84%
--------------------------------------------------------------
TOTAL                                        500     31    94%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
================== 39 passed, 2 skipped in 600.65s (0:10:00) ===================

Copy link
Collaborator

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

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

Looks good, but the question about using the list instead of an or statement.

For Google-Analytics-data-api, please check the flakeCheck errors https://github.com/airbytehq/airbyte/actions/runs/4947619403/jobs/8847290268#step:11:717

stream = GoogleAnalyticsDataApiMetadataStream(config=config, authenticator=config["authenticator"])
metadata = next(stream.read_records(sync_mode=SyncMode.full_refresh), None)
except HTTPError as e:
if e.response.status_code == HTTPStatus.BAD_REQUEST or e.response.status_code == HTTPStatus.FORBIDDEN:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not to use a list of errors here, instead, like:

if e.response.status_code  in [HTTPStatus.BAD_REQUEST, HTTPStatus.FORBIDDEN]:
    ....

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented May 12, 2023

/publish connector=connectors/source-google-ads

🕑 Publishing the following connectors:
connectors/source-google-ads
https://github.com/airbytehq/airbyte/actions/runs/4961191309


Connector Version Did it publish? Were definitions generated?
connectors/source-google-ads 0.2.17

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented May 12, 2023

/test connector=connectors/source-google-analytics-data-api

🕑 connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/4961883441
✅ connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/4961883441
Python tests coverage:

Name                                                Stmts   Miss  Cover
-----------------------------------------------------------------------
source_google_analytics_data_api/__init__.py            2      0   100%
source_google_analytics_data_api/authenticator.py      44      2    95%
source_google_analytics_data_api/utils.py              26      2    92%
source_google_analytics_data_api/source.py            237     28    88%
source_google_analytics_data_api/api_quota.py          94     12    87%
-----------------------------------------------------------------------
TOTAL                                                 403     44    89%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:109: Backward compatibility tests are disabled for version 0.2.1.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
============= 38 passed, 2 skipped, 1 warning in 431.86s (0:07:11) =============

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented May 12, 2023

/publish connector=connectors/source-google-analytics-data-api

🕑 Publishing the following connectors:
connectors/source-google-analytics-data-api
https://github.com/airbytehq/airbyte/actions/runs/4962045164


Connector Version Did it publish? Were definitions generated?
connectors/source-google-analytics-data-api 0.2.2

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@lazebnyi lazebnyi merged commit 1060415 into master May 12, 2023
@lazebnyi lazebnyi deleted the lazebnyi/categorize-source-google-ads-and-google-analytics-v4-config-error branch May 12, 2023 19:39
marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
…d Google Ads (airbytehq#25987)

* Categorized Config Errors Accurately

* Update PR number

* Update error list for GAv4

* Updated version

* Updated formating

* auto-bump connector version

* Skip spec backward compatibility

* auto-bump connector version

---------

Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Categorize Google Analytics 4 (GA4) config errors Categorize Google Ads config erros
3 participants