Skip to content

cdk: do not call init_uncaught_exception_handler from modules' root #14892

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 21, 2022
Merged

cdk: do not call init_uncaught_exception_handler from modules' root #14892

merged 6 commits into from
Jul 21, 2022

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Jul 20, 2022 •
edited
Loading

What

Closing #14453
Calling init_uncaught_exception_handler from the module root affects the stack trace of projects using the CDK's model.

>>> raise RuntimeError("hi")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: hi
>>> import airbyte_cdk
>>> raise RuntimeError("hi")
{"type": "LOG", "log": {"level": "FATAL", "message": "hi\nTraceback (most recent call last):\n  File \"<stdin>\", line 1, in <module>\nRuntimeError: hi"}}
{"type": "TRACE", "trace": {"type": "ERROR", "emitted_at": 1657117935965.3152, "error": {"message": "Something went wrong in the connector. See the logs for more details.", "internal_message": "hi", "stack_trace": "Traceback (most recent call last):\n  File \"<stdin>\", line 1, in <module>\nRuntimeError: hi\n", "failure_type": "system_error"}}}

How

To overcome this problem I moved the call to init_uncaught_exception_handler when real execution logic happens: in AirbyteEntrypoint.__init__ for sources and in Destination.run_cmd for destinations.

@alafanechere alafanechere requested a review from a team July 20, 2022 17:20
@github-actions github-actions bot added the CDK Connector Development Kit label Jul 20, 2022
@alafanechere alafanechere linked an issue Jul 20, 2022 that may be closed by this pull request
CDK: Only initialize uncaught exception handler when using AirbyteEntrypoint #14453
Closed
@alafanechere alafanechere requested a review from pedroslopez July 20, 2022 17:24
sherifnada
sherifnada approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

🔥 1 PR per day streak continues

pedroslopez
pedroslopez approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@pedroslopez pedroslopez left a comment

Choose a reason for hiding this comment

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

Nice! Just left a couple minor suggestions

airbyte-cdk/python/unit_tests/destinations/test_destination.py
@@ -136,6 +137,13 @@ def __eq__(self, other):


class TestRun:
def test_run_cmd(self, mocker, destination: Destination):
mocker.patch.object(destination_module, "init_uncaught_exception_handler")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be more appropriate to patch/assert on the exception_handler module directly instead of the method being exposed within Destination or AirbyteEntrypoint

Copy link
Contributor Author

@alafanechere alafanechere Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In the context of this test, I think it's a better practice to mock what's being used by the function under test: the init_uncaught_exception_handler object available in destination module namespace and not the actual function in the exception_handler module. This test is not a concern, but patching the function could lead to unexpected side effects. Moreover, it allows us to change the module in which init_uncaught_exception_handler is declared without refactoring the test 😄

@alafanechere
Copy link
Contributor Author

alafanechere commented Jul 21, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/2710301002
✅ https://github.com/airbytehq/airbyte/actions/runs/2710301002

Phlair
Phlair approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@Phlair Phlair left a comment

Choose a reason for hiding this comment

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

🚀

@alafanechere
Copy link
Contributor Author

alafanechere commented Jul 21, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/2712357534
✅ https://github.com/airbytehq/airbyte/actions/runs/2712357534

@alafanechere alafanechere merged commit b76b73b into master Jul 21, 2022
@alafanechere alafanechere deleted the augustin/cdk/uncaught-exception-handler-in-airbyte-entrypoint branch July 21, 2022 14:34
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Jul 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroslopez pedroslopez pedroslopez approved these changes

@sherifnada sherifnada sherifnada approved these changes

@Phlair Phlair Phlair approved these changes

Assignees
No one assigned
Labels
CDK Connector Development Kit team/extensibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CDK: Only initialize uncaught exception handler when using AirbyteEntrypoint
4 participants
@alafanechere @pedroslopez @sherifnada @Phlair