Add AirbyteTraceMessage to Airbyte protocol #12458

alovew · 2022-04-28T22:56:41Z

What

AirbyteTraceMessages is a new type of AirbyteMessage that will display information about syncs to users. The first use case is to display error messages, and in the future there will be other AirbyteTraceMessage types to display different kinds of information in a user friendly way. More information about the error use case is outlined in these two documents:
https://docs.google.com/document/d/1ctrj3Yh_GjtQ93aND-WH3ocqGxsmxyC3jfiarrF6NY0
https://docs.google.com/document/d/1nDVU-6_lDHkNVHPFHwfozaEoGl9hmV42RaAbv__6Bqo

sherifnada

nice! I would recommend we also add a blurb in the docs describing this new message https://docs.airbyte.com/understanding-airbyte/airbyte-specification

sherifnada · 2022-04-28T22:59:47Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

@@ -43,6 +44,9 @@ definitions:
      state:
        description: "schema message: the state. Must be the last message produced. The platform uses this information"
        "$ref": "#/definitions/AirbyteStateMessage"
+      trace:
+        description: "trace message: a message to provide human readable information about a sync"


it could also be about non-syncs like check and discover operations right?

yeah, you're right, I was going off of the spec but will update.

michel-tricot · 2022-04-29T04:10:26Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

@@ -43,6 +44,9 @@ definitions:
      state:
        description: "schema message: the state. Must be the last message produced. The platform uses this information"
        "$ref": "#/definitions/AirbyteStateMessage"
+      trace:
+        description: "trace message: a message to provide human readable information to users"


It is important to not call it human readable. It is really about providing structured information about the connector.

Perhaps alternative language might be:

"trace message: a structured message to share information about connector and sync performance with external consumers"

Or perhaps

"trace message: a structured message to communicate information about the status and performance of a connector"

I would be happy with either

michel-tricot · 2022-04-29T04:12:52Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+      - message
+    properties:
+      message:
+        description: The human readable message that will be shown to the user in the UI


Can you remove the mention of the UI? the protocol doesn't know anything about the UI.

For the protocol changes, it is important to not mix the what the data is with how it is being used.

Suggested change

description: The human readable message that will be shown to the user in the UI

description: A user-friendly message that indicates the cause of the error

Maybe using the wording user-friendly here is a good middle ground, as it indicates we want this to be understandable by users of the system, without referencing another part of the platform (UI)

michel-tricot · 2022-04-29T04:15:04Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+        description: The human readable message that will be shown to the user in the UI
+        type: string
+      internal_message:
+        description: The less human readable, more technical message showing the reason for the failure, e.g. the first line of a stack trace


you can probably remove the concept of human readability. Also, why do we need this field? The example you're giving mentions the stack grace so I assume we could just read the stack_trace field first line

I agree with you. Evan and I discussed this in the spec - @evantahler I think you were thinking that it's not necessarily going to be the first line of a stack trace and could be some other kind of message?

Looking at the conversation again - @evantahler maybe you only wanted this field for AirbyteTraceMessages that are not errors? https://docs.google.com/document/d/1ctrj3Yh_GjtQ93aND-WH3ocqGxsmxyC3jfiarrF6NY0/edit?disco=AAAAYr7ZeRQ

Maybe this is backwards thinking, but the inspiration for the message once again comes from our (future) usage of these messages to create FailureReasons, and we've got all 3: message, internal_message, and stack_trace in FailureReasons.

I do think there is value for message, internal_message, and stack_trace. An error like

message: API Key not accepted - check your credentials internal_message: 403: Access Denied stack_trace: "403: Access Denied" from connect() on line 101...

could come from a number of different places in the code (really, any time we make an HTTP request). Analyzing these by internal message (which will always be the same) even though they come from different stack traces will be helpful. Grouping by the full stack trace without doing some extraction won't be helpful. The "message" might be decorated better to say "...check your username" vs "... check your apiKey" if we are able to do that.

Also, I think the connector, using the same programming language that generated the stack trace, is the best place to "extract" the most meaningful message from the stack, rather than the platform/airbyte-worker trying to do it.

Based on Evan's comment I think internal_message makes sense to keep, but I agree that we should remove less human readable from the description, and probably also remove e.g. the first line of a stack trace. Intead this could more simple like The message of the internal error that caused the failure or something along those lines

michel-tricot

Nice :)

Can you just removed the concept of "human readable" from PR desc?

Trace message is really not about human readability. It is just about providing structured information about the connectors

evantahler · 2022-04-29T15:43:57Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

@@ -43,6 +44,9 @@ definitions:
      state:
        description: "schema message: the state. Must be the last message produced. The platform uses this information"
        "$ref": "#/definitions/AirbyteStateMessage"
+      trace:
+        description: "trace message: a message to provide human readable information to users"


Perhaps alternative language might be:

"trace message: a structured message to share information about connector and sync performance with external consumers"

evantahler · 2022-04-29T15:46:16Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+        "$ref": "#/definitions/AirbyteErrorTraceMessage"
+  AirbyteErrorTraceMessage:
+    type: object
+    additionalProperties: true


👍 on additionalProperties - The thought was that we might take any additional properties and append them to the FailureReason's "metadata" property.

cc @pedroslopez

recap from discussion on the doc: this will not be the case - additionalProperties is true so that it will be ok if a connector does erroneously set an additional property, and it's easier to make backwards-compatible changes to the protocol later.

No further properties are expected to be included in AirbyteErrorTraceMessage and the connector logs should give additional context.

evantahler · 2022-04-29T15:47:20Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+      failure_type:
+        description: The type of error
+        type: string
+        enum:
+          - SYSTEM_ERROR
+          - CONFIG_ERROR


Noting that these enum values and https://github.com/airbytehq/airbyte/blob/master/airbyte-config/models/src/main/resources/types/FailureReason.yaml#L20-L26 have different cases. Not sure if that matters

Since we will need to convert between the POJO type generated from this json schema to the type generated by the FailureReason json schema, it should be pretty trivial to lowercase these values when performing the conversion.

However, given that there are already other lowercase string enum values in this protocol file (e.g. SyncMode), we might as well make these enum values lowercase as well to have them be consistent with the FailureReason values.

lmossman

@alovew I left some comments with my feedback on these changes and other discussions. This looks very close! As Sherif suggested, we should also update the airbyte-specification.md docs file with a note describing this new message type added to The Airbyte Protocol section.

Once that is done and all of the open conversations here are addressed, this should be pretty much ready to go

lmossman · 2022-04-29T18:25:55Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

@@ -43,6 +44,9 @@ definitions:
      state:
        description: "schema message: the state. Must be the last message produced. The platform uses this information"
        "$ref": "#/definitions/AirbyteStateMessage"
+      trace:
+        description: "trace message: a message to provide human readable information to users"


Or perhaps

"trace message: a structured message to communicate information about the status and performance of a connector"

I would be happy with either

lmossman · 2022-04-29T18:42:25Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+      - type
+      - emitted_at
+    properties:
+      type:


I was going to leave a comment here suggesting that we use the json schema oneOf to implement the different types here instead of having a type property, and a separate property for each trace message type. However, I found this comment in our code base which indicates that jsonschema2pojo does not support oneOf. Since we use that to generate pojos for our platform, this means we can't use that here, so we'll have to stick with this type enum approach.

lmossman · 2022-04-29T18:45:45Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+        description: "the time in ms that the message was emitted"
+        type: number
+      error:
+        description: "error data from the sync"


Suggested change

description: "error data from the sync"

description: "error trace message: the error object"

Suggesting this description change to use similar wording as the AirbyteMessage's various sub-type descriptions, since we have a similar structure here

lmossman · 2022-04-29T18:54:19Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+      - message
+    properties:
+      message:
+        description: The human readable message that will be shown to the user in the UI


Suggested change

description: The human readable message that will be shown to the user in the UI

description: A user-friendly message that indicates the cause of the error

Maybe using the wording user-friendly here is a good middle ground, as it indicates we want this to be understandable by users of the system, without referencing another part of the platform (UI)

lmossman · 2022-04-29T19:00:06Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+        description: The human readable message that will be shown to the user in the UI
+        type: string
+      internal_message:
+        description: The less human readable, more technical message showing the reason for the failure, e.g. the first line of a stack trace


Based on Evan's comment I think internal_message makes sense to keep, but I agree that we should remove less human readable from the description, and probably also remove e.g. the first line of a stack trace. Intead this could more simple like The message of the internal error that caused the failure or something along those lines

lmossman · 2022-04-29T19:05:13Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+      failure_type:
+        description: The type of error
+        type: string
+        enum:
+          - SYSTEM_ERROR
+          - CONFIG_ERROR


Since we will need to convert between the POJO type generated from this json schema to the type generated by the FailureReason json schema, it should be pretty trivial to lowercase these values when performing the conversion.

However, given that there are already other lowercase string enum values in this protocol file (e.g. SyncMode), we might as well make these enum values lowercase as well to have them be consistent with the FailureReason values.

alovew · 2022-05-02T20:12:46Z

This is ready for re-review - just want to give everyone a chance to look at this again before merging.

lmossman

One last small suggestion, otherwise LGTM

docs/understanding-airbyte/airbyte-specification.md

Co-authored-by: Lake Mossman <lake@airbyte.io>

pedroslopez

LGTM 👍🏼

pedroslopez · 2022-05-02T23:55:18Z

airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+        "$ref": "#/definitions/AirbyteErrorTraceMessage"
+  AirbyteErrorTraceMessage:
+    type: object
+    additionalProperties: true


recap from discussion on the doc: this will not be the case - additionalProperties is true so that it will be ok if a connector does erroneously set an additional property, and it's easier to make backwards-compatible changes to the protocol later.

No further properties are expected to be included in AirbyteErrorTraceMessage and the connector logs should give additional context.

tuliren · 2022-05-04T01:09:50Z

The "Connectors Base" failed after this PR is merged. Looks like some of the auto generated files need to be updated. @alovew, can you take a look?

Update: never mind. It should be fixed.

* Add AirbyteTraceMessage to protocol Co-authored-by: Lake Mossman <lake@airbyte.io>

alovew added 2 commits April 28, 2022 14:47

Add AirbyteTraceMessage to protocol

d90e8be

add failure type

f6fab4d

github-actions bot added area/platform issues related to the platform area/protocol labels Apr 28, 2022

sherifnada reviewed Apr 28, 2022

View reviewed changes

remove manual cancellation

5948b31

alovew temporarily deployed to more-secrets April 28, 2022 23:57 Inactive

update trace message description

43f8194

alovew requested review from lmossman, evantahler and pedroslopez April 29, 2022 00:07

alovew temporarily deployed to more-secrets April 29, 2022 00:09 Inactive

michel-tricot reviewed Apr 29, 2022

View reviewed changes

Phlair mentioned this pull request Apr 29, 2022

[EPIC] All CDK Connectors emit AirbyteTraceMessage when something goes wrong #12322

Closed

4 tasks

evantahler reviewed Apr 29, 2022

View reviewed changes

lmossman reviewed Apr 29, 2022

View reviewed changes

code review changes and updating docs

69a732a

github-actions bot added the area/documentation Improvements or additions to documentation label Apr 29, 2022

alovew temporarily deployed to more-secrets April 29, 2022 21:59 Inactive

alovew temporarily deployed to more-secrets May 2, 2022 17:05 Inactive

grishick mentioned this pull request May 2, 2022

[EPIC] Improve database sources & destinations error messages #11322

Closed

evantahler approved these changes May 2, 2022

View reviewed changes

lmossman approved these changes May 2, 2022

View reviewed changes

docs/understanding-airbyte/airbyte-specification.md Outdated Show resolved Hide resolved

Tweak docs

1a905d3

Co-authored-by: Lake Mossman <lake@airbyte.io>

alovew temporarily deployed to more-secrets May 2, 2022 20:46 Inactive

pedroslopez approved these changes May 3, 2022

View reviewed changes

evantahler mentioned this pull request May 3, 2022

[EPIC] Ensure Source and Destination errors are distinguished and useful (Defogging phase 0) #12118

Closed

14 tasks

alovew merged commit 21c1364 into master May 3, 2022

alovew deleted the anne/airbyte-display-message branch May 3, 2022 21:11

octavia-squidington-iii mentioned this pull request May 4, 2022

Bump Airbyte version from 0.36.7-alpha to 0.36.8-alpha #12553

Merged

sherifnada mentioned this pull request May 5, 2022

Add AirbyteTraceMessage to the airbyte protocol #12519

Closed

suhomud pushed a commit that referenced this pull request May 23, 2022

Add AirbyteTraceMessage to Airbyte protocol (#12458)

33cdd0f

* Add AirbyteTraceMessage to protocol Co-authored-by: Lake Mossman <lake@airbyte.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AirbyteTraceMessage to Airbyte protocol #12458

Add AirbyteTraceMessage to Airbyte protocol #12458

alovew commented Apr 28, 2022

sherifnada left a comment

sherifnada Apr 28, 2022

alovew Apr 29, 2022

michel-tricot Apr 29, 2022

evantahler Apr 29, 2022

lmossman Apr 29, 2022

michel-tricot Apr 29, 2022

lmossman Apr 29, 2022

michel-tricot Apr 29, 2022

alovew Apr 29, 2022

alovew Apr 29, 2022

evantahler Apr 29, 2022 •

edited

Loading

lmossman Apr 29, 2022

michel-tricot left a comment

evantahler Apr 29, 2022

evantahler Apr 29, 2022

pedroslopez May 2, 2022

evantahler Apr 29, 2022

lmossman Apr 29, 2022

lmossman left a comment

lmossman Apr 29, 2022

lmossman Apr 29, 2022

lmossman Apr 29, 2022

lmossman Apr 29, 2022

lmossman Apr 29, 2022

lmossman Apr 29, 2022

alovew commented May 2, 2022

lmossman left a comment

pedroslopez left a comment

pedroslopez May 2, 2022

tuliren commented May 4, 2022 •

edited

Loading

	description: The human readable message that will be shown to the user in the UI
	description: A user-friendly message that indicates the cause of the error

	description: "error data from the sync"
	description: "error trace message: the error object"

Add AirbyteTraceMessage to Airbyte protocol #12458

Add AirbyteTraceMessage to Airbyte protocol #12458

Conversation

alovew commented Apr 28, 2022

What

sherifnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evantahler Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michel-tricot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alovew commented May 2, 2022

lmossman left a comment

Choose a reason for hiding this comment

pedroslopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliren commented May 4, 2022 • edited Loading

evantahler Apr 29, 2022 •

edited

Loading

tuliren commented May 4, 2022 •

edited

Loading