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

🎉 New Source: Monday #7168

Merged
merged 15 commits into from
Nov 6, 2021
Merged

Conversation

tuanchris
Copy link
Contributor

@tuanchris tuanchris commented Oct 19, 2021

What

How

Describe the solution

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions
  • Connector added to connector index like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions
  • Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

@tuanchris tuanchris marked this pull request as ready for review October 20, 2021 23:37
@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Oct 21, 2021

Hey @tuanchris thanks for the contribution. We will review it soon. Can you share the credentials over slack (DM) for me test it out.

.vscode/launch.json Outdated Show resolved Hide resolved
@marcosmarxm
Copy link
Member

Please share unit test and integration tests screen shoot showing they are passing locally.

@tuanchris
Copy link
Contributor Author

@marcosmarxm Here are the tests:
Unit test:
image

Integration test:
image
The Integration test is failing with the Freshsalse source:

INFO     detailed_logger acceptance_tests_logs/test_full_refresh.py__TestFullRefresh__test_sequential_reads[inputs0].txt:test_full_refresh.py:25 The two sequential reads should produce either equal set of records or one of them is a strict subset of the other

I tried looking at the two output file:
run 1:
image
run 2:
image
For some reason, there is no stream boards in run 2. Any idea what may cause this?

@tuanchris
Copy link
Contributor Author

The issue was due to an uncaught retry problem. Fixed that and the local integration test is passing now.

image

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

Please see comments

Comment on lines +51 to +55
def should_retry(self, response: requests.Response) -> bool:
# Monday API return code 200 with and errors key if complexity is too high.
# https://api.developer.monday.com/docs/complexity-queries
is_complex_query = response.json().get("errors")
return response.status_code == 429 or 500 <= response.status_code < 600 or is_complex_query
Copy link
Member

Choose a reason for hiding this comment

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

This will request the same query? If its a complex query this means will keep in a loop?

Copy link
Member

Choose a reason for hiding this comment

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

I ran integration tests and got backoff error, I need to add a logging to understand the error:

2021-10-26 01:10:13 INFO () DefaultAirbyteStreamFactory(lambda$create$0):53 - [{'message': "Field 'users' doesn't accept argument 'page'", 'locations': [{'line': 1, 'column': 16}], 'fields': ['query', 'users', 'page']}]
2021-10-26 01:10:13 INFO () DefaultAirbyteStreamFactory(internalLog):86 - Backing off _send(...) for 15.0s (airbyte_cdk.sources.streams.http.exceptions.DefaultBackoffException)
2021-10-26 01:10:29 INFO () DefaultAirbyteStreamFactory(internalLog):86 - Caught retryable error '' after 1 tries. Waiting 15 seconds then retrying...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this document, the free trial account (which we are on) can only call queries with 1 mil or less per minute. In case of integration test, I think we reached the limit and this is the response.txt from the request:

{"errors":[{"message":"Complexity budget exhausted, query cost 202535 budget remaining 189860 out of 1000000 reset in 43 seconds"}],"account_id":10204492}

The code above is meant to check if status_code == 200 and there is an errors key in the response.json()

Copy link
Member

Choose a reason for hiding this comment

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

Got it, the question is: if a query can stay in Complex Query and always keep in backoff.

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 don't think this is possible. From the same link: Complexity levels are calculated based on the query.. If a query return more than the complexity limit, we couldn't do the initial sync. The limit was hit because, with the integration test, we are running many successive queries in a short period of time. The retry and backoff will handle that.

…urce.py

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
@marcosmarxm marcosmarxm self-assigned this Nov 1, 2021
@marcosmarxm marcosmarxm temporarily deployed to more-secrets November 6, 2021 20:16 Inactive
Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

thanks @tuanchris

@marcosmarxm marcosmarxm merged commit 0b9b8ba into airbytehq:master Nov 6, 2021
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
* add items stream

* add boards stream

* add remaining streams

* check point

* lint

* fix unit test & linting

* Delete launch.json

* fix schema

* add retry logic

* Update airbyte-integrations/connectors/source-monday/source_monday/source.py

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>

* small fix + add creds

* bump version and config file

Co-authored-by: Marcos Marx <marcosmarxm@gmail.com>
Co-authored-by: Marcos Marx <marcosmarxm@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.

6 participants