AirbyteLib: Exception Handling

[aaronsteers]

This brings more formalized exception handling to AirbyteLib.

This handles the lint rules:

• https://docs.astral.sh/ruff/rules/#tryceratops-try
• https://docs.astral.sh/ruff/rules/raise-vanilla-class/
• https://docs.astral.sh/ruff/rules/raise-vanilla-args/

More detail in the new exceptions module. From the file docstring:

This design is modeled after structlog's exceptions, in that we bias towards auto-generated
property prints rather than sentence-like string concatenation.

E.g. Instead of this:

Subprocess failed with exit code '1'

We do this:

Subprocess failed. (exit_code=1)

The benefit of this approach is that we can easily support structured logging, and we can
easily add new properties to exceptions without having to update all the places where they
are raised. We can also support any arbitrary number of properties in exceptions, without spending
time on building sentence-like string constructions with optional inputs.

In addition, the following principles are applied for exception class design:

• All exceptions inherit from a common base class.
• All exceptions have a message attribute.
• The first line of the docstring is used as the default message.
• The default message can be overridden by explicitly setting the message attribute.
• Exceptions may optionally have a guidance attribute.
• Exceptions may optionally have a help_url attribute.
• Rendering is automatically handled by the base class.
• Any helpful context not defined by the exception class can be passed in the context dict arg.
• Within reason, avoid sending PII to the exception constructor.
• Exceptions are dataclasses, so they can be instantiated with keyword arguments.
• Use the 'from' syntax to chain exceptions when it is helpful to do so.
  E.g. raise AirbyteConnectorNotFoundError(...) from FileNotFoundError(connector_path)
• Any exception that adds a new property should also be decorated as @dataclass.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jan 24, 2024 9:33pm

[aaronsteers]

@flash1293 - A few tests are broken but I wanted to open this for your review and for our discussion on the design proposals here.

[flash1293]

Love the design - this is set up really well for the future! I started leaving nits of which information should be provided in which place and the semantics of different error classes, but I will hold off for now as it seems like you are still fixing.

[flash1293]

should we add stdout/stderr and args as context here?

[aaronsteers]

I left it out to avoid risk - and with a focus on parity. But yes, definitely we can add in the future. 👍

[flash1293]

This specific branch is actionable - we can give a guidance of either install yourself or set the install_if_missing option

[aaronsteers]

Added!

[aaronsteers]

@flash1293 - Thanks for the review! I will incorporate your suggestions. Just as fyi, in some cases, I knew I could add more context but I was trying to limit scope - with the objective of getting parity without adding potential for new bugs/issues.

One nice thing about this model is that it's easily extensible. We can find new attributes to log in the exception and add them incrementally without much added effort. I imagine as we see these raised 'in the wild', we'll see opportunities to improve the debugging experience for us and users.

I've also fixed all tests - so you can complete the review if you had anything you wanted to add.

This PR fixes an unrelated bug that was preventing tests from passing. I've merged it into this branch so that tests are green.

• AirbyteLib: pin pandas to 2.1.4 #34491

[aaronsteers]

@flash1293 - One thing to add here - a big advantage of these custom classes will be the ability to tie the exception name and message into telemetry without worrying about PII. Sometimes paths and text inputs will have user entered info, but in this implementation, we can safely expect that message, class name, and call stack file/line-nums are always safe to report.

We can also add specific attributes as safe for telemetry, such as 'exit_code', which will never have PII because it is an integer.

[aaronsteers]

@flash1293 - This resolves most feedback, but not 100%. Specifically, I opted not to add stdout/stderr prints for now due to needing to retest and/or add new tests if so. We can keep adding from here though, as discussed.

Merging now that tests pass, so we can keep things moving. :)