CDK: remove unexpected error swallowing on abstract source's check method
#24240
Conversation
octavia-squidington-iii
added
area/connectors
|
/test connector=connectors/source-airtable local_cdk=true connector-acceptance-test-version=dev
Build FailedTest summary info: |
Affected Connector ReportNOTE
|
| Connector | Version | Changelog | Publish |
|---|---|---|---|
source-railz |
0.1.1 |
✅ | ✅ |
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Destinations (0)
| Connector | Version | Changelog | Publish |
|---|
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Other Modules (0)
Actionable Items
(click to expand)
| Category | Status | Actionable Item |
|---|---|---|
| Version | ❌ mismatch |
The version of the connector is different from its normal variant. Please bump the version of the connector. |
| ⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
|
| Changelog | ⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
| ❌ changelog missing |
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog. | |
| Publish | ⚠ not in seed |
The connector is not in the seed file (e.g. source_definitions.yaml), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug. |
| ❌ diff seed version |
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version. |
|
@clnoll I think I'm hitting a edge case while trying to use local CDK and local CAT. When I pass the local_cdk flag the |
|
Thanks for the heads up @alafanechere, fix is incoming in a few. |
|
The fix is merged @alafanechere. |
|
/test connector=connectors/source-airtable local_cdk=true connector-acceptance-test-version=dev
Build PassedTest summary info: |
There was a problem hiding this comment.
Thank you for taking this on! Code LGTM, I only wonder about other connectors that might start failing the test and how we want to handle them.
Would also probably be good to get a list of CDK connectors that override check instead of overriding check_connection, since they won't inherit the change - not that we have to fix them here, but would be good to have a list for e.g. GL to look into
| elif inputs.status == ConnectionTestConfig.Status.Exception: | ||
| with pytest.raises(ContainerError) as err: | ||
| docker_runner.call_check(config=connector_config) | ||
|
|
||
| assert err.value.exit_status != 0, "Connector should exit with error code" | ||
| assert "Traceback" in err.value.stderr, "Connector should print exception" | ||
| output = docker_runner.call_check(config=connector_config, raise_container_error=False) | ||
| trace_messages = filter_output(output, Type.TRACE) | ||
| assert len(trace_messages) == 1, "A trace message should be emitted in case of unexpected errors" | ||
| trace = trace_messages[0].trace | ||
| assert isinstance(trace, AirbyteTraceMessage) | ||
| assert trace.error is not None | ||
| assert trace.error.message is not None | ||
| assert trace.error.internal_message is not None |
There was a problem hiding this comment.
👍🏻 I like this test because it doesn't make any opinions about whether a connection_status message is output, only confirms that a trace message is output. This follows the behavior that I now use in the platform, where if there was a trace message, we declare that there was an error and don't take a connection_status message (if existing) into account. So connectors that currently output both won't fail the test.
Are we concerned about connectors that aren't CDK ones that have the same behavior (e.g. most destinations - not sure if we regularly run CAT on destinations but we could) will suddenly start failing this?
There was a problem hiding this comment.
I guess this is only tested if someone provides a status: "exception" config to test though
There was a problem hiding this comment.
I guess this is only tested if someone provides a status: "exception" config to test though
Yes indeed.
I'd love to find a way to create a global not configurable test for this. Because it's unlikely developers will fill in a status: "exception" as they'll rather use status: "fail" to make sure the expected errors are correctly handled.
Do you have a solution in mind to always make check fail in this test context? (Maybe preventing internet access to the container under test?)
There was a problem hiding this comment.
I believe check currently fails if we pass in a path to a config that doesn't exist (this happens all the time if you haven't written the secrets to storage). We could try to leverage that?
There was a problem hiding this comment.
E.g. I just ran this locally:
/var/folders/q9/zbmmxqk11tv_jbvdjs428gzm0000gp/T ─╮
❯ docker run airbyte/source-drift:dev check --config blah.txt ─╯
{"type": "LOG", "log": {"level": "FATAL", "message": "[Errno 2] No such file or directory: 'blah.txt'\nTraceback (most recent call last):\n File \"/airbyte/integration_code/main.py\", line 13, in <module>\n launch(source, sys.argv[1:])\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/entrypoint.py\", line 131, in launch\n for message in source_entrypoint.run(parsed_args):\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/entrypoint.py\", line 85, in run\n raw_config = self.source.read_config(parsed_args.config)\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/connector.py\", line 51, in read_config\n config = BaseConnector._read_json_file(config_path)\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/connector.py\", line 61, in _read_json_file\n with open(file_path, \"r\") as file:\nFileNotFoundError: [Errno 2] No such file or directory: 'blah.txt'"}}
{"type": "TRACE", "trace": {"type": "ERROR", "emitted_at": 1679409920509.809, "error": {"message": "Something went wrong in the connector. See the logs for more details.", "internal_message": "[Errno 2] No such file or directory: 'blah.txt'", "stack_trace": "Traceback (most recent call last):\n File \"/airbyte/integration_code/main.py\", line 13, in <module>\n launch(source, sys.argv[1:])\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/entrypoint.py\", line 131, in launch\n for message in source_entrypoint.run(parsed_args):\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/entrypoint.py\", line 85, in run\n raw_config = self.source.read_config(parsed_args.config)\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/connector.py\", line 51, in read_config\n config = BaseConnector._read_json_file(config_path)\n File \"/usr/local/lib/python3.9/site-packages/airbyte_cdk/connector.py\", line 61, in _read_json_file\n with open(file_path, \"r\") as file:\nFileNotFoundError: [Errno 2] No such file or directory: 'blah.txt'\n", "failure_type": "system_error"}}}
There was a problem hiding this comment.
Yep this could work. The problem is that test_check relies on the connector_config fixture which is reading the config file. Passing an unexisting config leads to a FileNotFoundError on CAT.
I'll try to create a separate test not using this fixture.
There was a problem hiding this comment.
Haha, our fixtures are too good to make fake errors 😄
| ## 0.7.2 | ||
| TestConnection: assert that a check with `exception` status emits a trace message. | ||
|
|
There was a problem hiding this comment.
Wondering if this should be bumped differently, as it's a new test we expect some to fail 🤔
There was a problem hiding this comment.
Only two connectors are using status: "exception" in their acceptance test config (dixa and file). They both are not passing CAT at the moment.
I don't expect additional connectors to fail as only these connectors + airtable will go into the inputs.status == ConnectionTestConfig.Status.Exception: statement.
There was a problem hiding this comment.
Makes sense, thanks for clarifying 👍🏻
...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py
Outdated
Show resolved
Hide resolved
|
@erohmensing here's the list of source connectors overriding the I created an issue to fix these connectors: https://github.com/airbytehq/airbyte-internal-issues/issues/1538 |
|
@erohmensing I can't find an obvious way to make all the connectors specifically fail at |
There was a problem hiding this comment.
I think the concern of failing tests has been raised by Ella. I'm wondering if there is more problem this compatibility change might incur. In theory, this is a breaking change (a user expecting an AirbyteConnectionStatus will now receive an exception) even though I understand it's probably "soft"
airbyte-cdk/python/CHANGELOG.md
Outdated
| @@ -1,5 +1,8 @@ | |||
| # Changelog | |||
|
|
|||
| ## 0.31.2 | |||
There was a problem hiding this comment.
This can be done as part of [the CDK release process] to avoid clash with other branches. Same with the changes in airbyte-cdk/python/setup.py
|
@maxi297 FWIW, the breaking part of this is handled so long as you are using airbyte's platform, as I changed the platform to be able to raise trace messages as errors to the user during the check step. When using it via docker, though, I do understand that the behavior changes - however, in docker you don't need the output of check in order to proceed to to the next step (so if you were ignoring that you got a failed check connection and moved on, you still can). The only way I could see this potentially changing someone's workflow significantly is if they had their own UI 🤔 I don't have very strong opinions on the versioning here, I'd be happy with bumping it more too, either one works for me |
|
@alafanechere after our chat yesterday I agree about not being able to test for unexpected errors. I think you're right - it probably makes more sense to sunset this option for the test and edit the connector code so that the 2 using "exception" configs lead to "failed" status indicating to the user that something is missing from their config. Id propose we create an issue to do this in a separate pr |
|
/test connector=connectors/source-amazon-ads local_cdk=true
Build PassedTest summary info: |
|
/test connector=connectors/source-paypal-transaction local_cdk=true
Build PassedTest summary info: |
|
/test connector=connectors/source-google-ads local_cdk=true
Build PassedTest summary info: |
|
/test connector=connectors/source-pinterest local_cdk=true
Build PassedTest summary info: |
|
/test connector=connectors/source-zendesk-talk local_cdk=true connector-acceptance-test-version=dev
Build PassedTest summary info: |
|
/test connector=connectors/source-google-analytics-v4 local_cdk=true connector-acceptance-test-version=dev
Build FailedTest summary info: |
|
/test connector=connectors/source-amazon-ads local_cdk=true connector-acceptance-test-version=dev
Build PassedTest summary info: |
|
/test connector=connectors/source-google-ads local_cdk=true connector-acceptance-test-version=dev
Build PassedTest summary info: |
|
/test connector=connectors/source-pinterest local_cdk=true connector-acceptance-test-version=dev
Build PassedTest summary info: |
|
/test connector=connectors/source-paypal-transaction local_cdk=true connector-acceptance-test-version=dev
Build PassedTest summary info: |
|
For google-analytics-v4 the same tests are failing on master so I can say that the failures are not related to my current changes. |
|
/publish connector=bases/connector-acceptance-test auto-bump-version=false
if you have connectors that successfully published but failed definition generation, follow step 4 here |
|
/test connector=connectors/source-marketo local_cdk=true
|
|
/test connector=connectors/source-alloydb local_cdk=true
Build FailedTest summary info: |
|
As Marketo does not finish I tested locally only the |
|
@erohmensing @maxi297 I'll merge it now that the impacted GA connectors are not likely to fail at CAT after their acceptance test config updates.
|
|
Thank you @alafanechere ! |
What
Closes #23281
We don't want to make unexpected error in
checkswallowed into a failed Connection Status. We want these error to be handled at a higher level with theuncaught_exception_handler, to emit trace messages.How
checkmethod of the AbstractSourceacceptance-test-config.ymlto assess the correct behavior of the implementation.Recommended reading order
airbyte/airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py
Line 83 in 578561a
airbyte/airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py
Line 490 in 578561a
🚨 User Impact 🚨
checkwill emit trace message in case of failure. (This is what we want)