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

🎉 Google Analytics connector is failing #3648

Merged
merged 21 commits into from
Jun 15, 2021

Conversation

po3na4skld
Copy link
Contributor

@po3na4skld po3na4skld commented May 26, 2021

What

It solves #3355.

How

Added filter into the GoogleAnalyticsSingerSource._get_reports_file_path so that it could remove non used in catalog provided by user. Also removed duplicated "title" key in spec.json file.

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. source.py
  2. spec.json

@po3na4skld po3na4skld added this to the Connectors May 28th, 2021 milestone May 26, 2021
@po3na4skld po3na4skld self-assigned this May 26, 2021
@po3na4skld po3na4skld linked an issue May 26, 2021 that may be closed by this pull request
Copy link
Contributor

@yevhenii-ldv yevhenii-ldv left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -59,19 +61,26 @@ def _validate_custom_reports(self, custom_reports_data: List[dict]):
error_messages.append(error.message)
raise Exception("An error occurred during custom_reports data validation: " + "; ".join(error_messages))

def read_catalog(self, catalog_path: str) -> ConfiguredAirbyteCatalog:
Copy link
Contributor

Choose a reason for hiding this comment

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

this method is not called anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is called when performing read operation, I redefined this function from a base class to save and then filter out streams we want to use

Copy link
Contributor

Choose a reason for hiding this comment

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

@po3na4skld, This method is called during the Read method, and when the Discover method is launched, no one stream will found, so we will get zero streams and we will not be able to read the data.

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 got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yevhenii-ldv for now it works as before

Copy link
Contributor

@yevhenii-ldv yevhenii-ldv left a comment

Choose a reason for hiding this comment

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

See above in comments

def _get_reports_file_path(self, temp_dir: str, custom_reports_data: List[dict]) -> str:
report_definition = (
json.loads(pkgutil.get_data("tap_google_analytics", "defaults/default_report_definition.json")) + custom_reports_data
)
if self.reports_to_read is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

if self.reports_to_read

Copy link
Contributor

@yevhenii-ldv yevhenii-ldv left a comment

Choose a reason for hiding this comment

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

LGTM, one small comment

@keu
Copy link
Contributor

keu commented Jun 3, 2021

@po3na4skld I see some strange files in PR that don't belong here, could you please merge with master and ensure there are no extra files.

@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 3, 2021

@po3na4skld I see some strange files in PR that don't belong here, could you please merge with master and ensure there are no extra files.

I think it is because I did chmod 777 on airbyte folder and after gradlew format they were added to the commits @keu you can see that there are not actual changes to these files. Merged master in this branch

Copy link
Contributor

@keu keu left a comment

Choose a reason for hiding this comment

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

please use this snippet to rollback changes
https://gist.github.com/liitfr/86c77bf0ea4a627ab9b62ad30c32c251

@po3na4skld
Copy link
Contributor Author

@davinchia
I added tests, but couldn't run them as the API tells that Daily Request Limit is reached and throws 429 HTTP error. The same when I run the read operation. I suppose I need to move this into blocked

@davinchia
Copy link
Contributor

thanks @po3na4skld

@sherifnada is this related to #3958? Have we tried getting our limit increased?

@po3na4skld
Copy link
Contributor Author

thanks @po3na4skld

@sherifnada is this related to #3958? Have we tried getting our limit increased?

@davinchia As I know for now we disabled any GA testing when merging to master in order to keep rate limit on an acceptable level for testing this PR

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

@po3na4skld thanks for adding the test!

since we can't test this; I think it's fine to merge if you feel confident about it. we should make sure we eventually come back and turn it the tests on so we have confidence things are working well.

@VasylLazebnyk VasylLazebnyk removed this from the Connectors June 11, 2021 milestone Jun 11, 2021
@keu
Copy link
Contributor

keu commented Jun 11, 2021

/test connector=source-googleanalytics-singer

🕑 source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/929608854
❌ source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/929608854

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Jun 15, 2021
@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 15, 2021

/publish connector=connectors/source-googleanalytics-singer

🕑 connectors/source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/938672052
❌ connectors/source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/938672052

@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 15, 2021

/test connector=connectors/source-googleanalytics-singer

🕑 connectors/source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/938979005
✅ connectors/source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/938979005

@yevhenii-ldv
Copy link
Contributor

yevhenii-ldv commented Jun 15, 2021

/publish connector=connectors/source-googleanalytics-singer

🕑 connectors/source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/939421703
✅ connectors/source-googleanalytics-singer https://github.com/airbytehq/airbyte/actions/runs/939421703

@po3na4skld po3na4skld dismissed sherifnada’s stale review June 15, 2021 15:46

He passed it to another person: davinchia

@po3na4skld po3na4skld merged commit 856abc3 into master Jun 15, 2021
@po3na4skld po3na4skld deleted the KC-Google-Analytics-Connector-is-failing branch June 15, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Analytics Connector is failing
6 participants