-
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
Use LOG4J2 to wrap connectors logs to JSON format #15668
Conversation
/test connector=connectors/source-postgres
Build FailedTest summary info:
|
With these changes, all connectors logs look like this:
We can configure this structure using |
45801d1
to
722593b
Compare
/test connector=connectors/source-mysql
Build PassedTest summary info:
|
722593b
to
9a36079
Compare
/test connector=connectors/source-postgres
Build PassedTest summary info:
|
9a36079
to
3198884
Compare
/test connector=connectors/source-postgres
Build PassedTest summary info:
|
@yurii-bidiuk could you show an example of what an exception stacktrace looks like? (i.e. if the connector has a And is https://github.com/airbytehq/airbyte-internal-issues/issues/19 an example of how the logs show up in the UI? (if yes: that looks correct to me - airbyte technically expects connectors to only output JSON, so the current behavior is actually violating the airbyte protocol) |
...rs/source-postgres/src/main/java/io/airbyte/integrations/source/postgres/PostgresSource.java
Outdated
Show resolved
Hide resolved
3198884
to
1b57642
Compare
@edgao
The original stack trace with a message is present in the sync summary log. |
Does the original stacktrace never get logged? That seems problematic. Losing the original stacktrace will make debugging connector crashes a lot more difficult. https://logging.apache.org/log4j/2.x/manual/json-template-layout.html says there are ways to add the exception stacktrace (the |
@edgao the original stack trace is logged only inside sync summary log.
You're right and I've already tried this config, but it's not working for Airbyte and stack trace is ignored. |
I'd pretty strongly prefer to have the exception stacktrace logged immediately, rather than captured inside the sync summary log. Otherwise it could hide important information (e.g. timestamp). |
NOTE
|
4f44a0b
to
eaa7704
Compare
NOTE
|
@edgao for some reason stack trace wasn't logged inside AirbyteExceptionHandler. So I manually added a logging stack trace as part of message and now we can see the original connector stack trace as part of the JSON log.
Can we move on with this? |
does this mean that anywhere else we use the also @yurii-bidiuk @grishick feel free to tell me I'm being too nitpicky here :P I wonder if we should just have a "stacktrace" field at the protocol level. Could be useful as a way to distinguish between technical vs non-technical error message? |
This does not sound good.
This seems like a good idea tagging @airbytehq/protocol-reviewers to chime in |
It is probably right to say that we shouldn't be "plain" logging exceptions anywhere, and that we should have some sort of formatting utility class. In that utility, we can decide (in a single method) if we |
aren't trace messages more "this is why the sync failed"? I think there's cases where we catch an exception but don't fail and still want to log it (e.g.
what do you mean by this? Like require that instead of using a log4j Logger, connector devs need to use some wrapper around it? |
Ah! We are talking about non-fatal exceptions (because we catch them and retry). I had initially assumed that exceptions were always fatal, and if that's the case, then we should be using AirbyteTraceMessage. If there are non-fatal exceptions, you can still send multiple AirbyteTraceMessage - if there are multiple AirbyteTraceMessage, it's safe to send multiple - the platform will use only the latest AirbyteTraceMessage as the main FailureReason. I guess the question is if the connector wants to "elevate" the information from those retryable exceptions to the platform or not.
Yep. I would suggest that there's a helper like: const e = new Error("oh no");
airbyte.utils.logException(e); // yes, a new custom method
//
function logError(Exception e, Boolean toSendTraceMessage = true) {
LOGGER.log("error", ExceptionUtils.getStackTrace(e));
if (toSendTraceMessage) { emitAirbyteTraceMessage(e) }
} CC @alovew |
This PR addresses both fatal and non-fatal exceptions, I'm just thinking through consequences for both cases. The fatal case is probably simple, since we can add a top-level try-catch to wrap it inside whatever we want. The non-fatal case is more interesting, because it requires code change everywhere non-fatal exceptions happen. My concern with adding a new method is that we can't really enforce that it's used everywhere. E.g. the official BigQuery SDK (used in destination-bigquery) has an internal exception retry handler, which we have no (reasonable) way of modifying to use the airbyte exception logger util. In that case we basically must rely on log4j logs working properly. (and yes, in that example, either we eventually succeed and therefore don't care about the retries, or it fails and throws a real exception. But I think there are cases where we want to see the exception log without it actually being fatal) |
NOTE
|
…bytehq/airbyte into bidiuk/659-wrap-logs-to-json
NOTE
|
…bytehq/airbyte into bidiuk/659-wrap-logs-to-json
NOTE
|
/publish connector=connectors/destination-mysql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/destination-postgres
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/destination-mssql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
@grishick @edgao |
NOTE
|
* master: (200 commits) 🪟 🧹 Display returned error messages on replication view (#16280) 🎉 Source mixpanel: Use "Retry-After" header for backoff (#16770) 🐛 Source google ads: mark custom query fields required (#15858) 🪟 🔧Remove useRouter hook (#16598) CDK: improve TypeTransformer to convert simple types to array of simple types (#16636) CDK: TypeTransformer - warning message more informative (#16695) Source MySQL: Add Python SAT to detect backwards breaking changes (#16445) remove eager (#16756) bump com.networknt:json-schema-validator to latest version (#16619) Remove Cloud from Kafka docs (#16753) Normalization Summaries table and read/write methods (#16655) comment out flaky test suite while it is being investigated (#16752) Update ConfigRepository to read protocol version (#16670) Use LOG4J2 to wrap connectors logs to JSON format (#15668) Update connector catalog (#16749) 🪟 🎨 Remove feedback modal from UI (#16548) Add missing env var for Kube overlays (#16747) Prepare for React v18 upgrade (#16694) 🪟 🐛 Fix direct job linking to work with pagination (#16517) Fix formatting (#16743) ...
* Use LOG4J2 to wrap connectors logs to JSON format * log connector's stack trace directly as a message * add stack_trace field to json template * bump versions * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * revert versions for destinations: postgres, mssql, mysql Co-authored-by: Octavia Squidington III <[email protected]>
* Use LOG4J2 to wrap connectors logs to JSON format * log connector's stack trace directly as a message * add stack_trace field to json template * bump versions * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] * revert versions for destinations: postgres, mssql, mysql Co-authored-by: Octavia Squidington III <[email protected]>
What
Fixes airbytehq/airbyte-internal-issues#19
How
Use LOG4J2 and JsonTemplateLayout to wrap connectors logs in JSON format like AirbyteLogMessage
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.