-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
return better structured error logs to connector builder #46963
Changes from all commits
850b682
d3f0a4b
eccf502
0200577
c0d6c24
2ede4dc
818e843
a4946f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,8 @@ class StreamReadSlices: | |
class LogMessage: | ||
message: str | ||
level: str | ||
internal_message: Optional[str] = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The trade off here is that since this does not map to the protocol error message model, when we move to using source-declarative-manifest container as manifest runner for Builder, we'd need to make sure that the protocol has these fields, otherwise error reporting will break. Should we make the work and align these models in this file with protocol-level models now instead? Or ship the small improvement and record a todo for later? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats a good callout Natik! Looking into it, this does already align with the protocol model ❓ @lmossman can you confirm Im reading this as this is already a protocol blessed type? If so should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This Even if we swap out the runtime to use source-declarative-manifest to execute the read, we will still need all of this extra If we don't want to directly execute any python anymore at all and therefore delete this So, I think I'd prefer to keep this PR simple right now, especially because I don't think we can easily reference the protocol AirbyteLogMessage and AirbyteTraceMessage in our connector-builder-server and airbyte-server openapi specs, and I'd rather not repeat those entire schemas as they are more complicated than the LogMessage schema I've defined here. But let me know if anything I said above sounds off @natikgadzhi @bnchrch ! |
||
stacktrace: Optional[str] = None | ||
|
||
|
||
@dataclass | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,4 +45,7 @@ def parse_response_error_message(self, response: requests.Response) -> Optional[ | |
body = response.json() | ||
return self._try_get_error(body) | ||
except requests.exceptions.JSONDecodeError: | ||
return None | ||
try: | ||
return response.content.decode("utf-8") | ||
except Exception: | ||
return None | ||
Comment on lines
+48
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change allows this method to also properly handle response bodies which are just string values, since that currently causes a JSONDecodeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Can we modify
AbstractSource
to indicate its a "Final Exception"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnchrch Do you have a specific approach in mind? We could set
internal_message
to something for example to indicate this but this would still rely on comparing strings in that approach.I'm wary to raise a different exception in abstract_source here because I'm not sure what relies on this being an AirbyteTracedException and I don't feel familiar enough with that fairly central CDK code to know what is safe to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add that any raised AirbyteTracedException is sort of already implied to be the final exception. Within
abstract_source.py
, https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py#L171-L177 , during a read of a stream always try to catch any form of Exception or AirbyteTracedException thrown by a stream and wrap it into an emitted trace message. We only formally raise the "final" AirbyteTracedException because we need to end the process with a non-zero error code.Granted the small gap is if we get an exception outside of that try/except block. But from my perspective, us trying to adjust the protocol or create new abstractions to capture what is effectively a work around feels like we're trying to over engineer a bit. The ideal design is that the platform can identify that merely receiving any AirbyteTracedException is enough to know that the sync was unsuccessful. And then we could get rid of this work around.
So TLDR: I think how we do it here is fine.