Skip to content
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

Incomplete events schema #1388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

colmsnowplow
Copy link
Contributor

No description provided.

},
"type": "object",
"properties": {
"source": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"errors": {
"type": "array",
"items": {
"type": "object"
Copy link
Contributor Author

@colmsnowplow colmsnowplow Feb 14, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially the idea was to have defined fields, but we changed it to just an empty object because of how BQ will load data - any defined fields will create a static definition of the object, which is a pain to change, and additional properties would be ignored. An empty object, however, will just load what it gets.

This isn't necessarily the ideal scenario. We should consider separate fields for error message (array of strings), and error details (flexible object), but that design has downsides too.

"description": "Source of the error. If the failure happened during enrichment, it'll be the enriched field - for example `user_ipaddress`, `unstruct` or `contexts`"
},
"failureType": {
"type": "string"
Copy link
Member

Choose a reason for hiding this comment

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

enum for safer usage, wdyt @colmsnowplow ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No objection in principal, but need to validate that changing the enum in future won't result in any problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Enum can still be a little awkward with Redshift as I don't believe we bump the maxLength as of yet.

"version": "1-0-0"
},
"type": "object",
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add something like an error_code. At the moment we put a lot of description elements into the failed event schemas but these aren't apparent as to what the failure relates to unless you actually add it into the schema. The way I've been thinking about this is to provide a high level classification e.g.,

D - the failure is related to data that has been sent with the event
S - the failure is related to an issue with a schema sent with the event
I - the failure is related to an internal processing error (e.g., pipeline issue)
E - the failure is related to an external processing issue (e.g., failed to get a response from an API enrichment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Mike. Interesting suggestion. @stanch would love to hear what you think.

For incomplete events I'd be inclined to treat it as post-MVP, but depending on how difficult to implement it might be something we could do before rolling out the end product

Copy link
Contributor Author

@colmsnowplow colmsnowplow Mar 11, 2024

Choose a reason for hiding this comment

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

@miike now that we have some firmer examples of what the actual data looks like, we tweaked some things. I think something like the requirement you describe will be satisfied by the failureType field.

This will essentially indicate the classification of the error: eg. validation error or schema violation, for example.

(additionally we're releasing 4.2.0 before this one, which fixes things so that schema violations at enrichment level don't get reported as enrichment errors.)

I'm not sure if this would give you everything you're asking for here, so we can still consider it post-MVP

Comment on lines 39 to 45
"producingAppName": {
"type": "string",
"description": "Name of the app which produced the failure"
},
"producingAppVersion": {
"type": "string",
"description": "Version of the app which produced the failure"
Copy link

Choose a reason for hiding this comment

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

I'm confused, are these trackers? App_ids? Or the part of the pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Component of the pipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the confusion though. Worth considering alternative names I guess. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

component / component_version? I think we call this artifact at the moment but component is likely more clear.

"description": "Version of the app which produced the failure"
}
},
"required": ["errors", "timestamp", "producingAppName", "producingAppVersion"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add failureType here

@colmsnowplow
Copy link
Contributor Author

colmsnowplow commented Mar 11, 2024

The most recent commits, following from a meeting to discuss design after seeing example failure data:

  • Define failureType as the classification of the failure, rather than anything to do with where the failure came up (eg. validation error)
  • removes source - source is to indicate the field which caused the issue, but this is actually specific to each error rather than general for the whole failure. For example one can have a failure where there is an error for page_url as well as page_referrer.
  • Renames unclear fields
  • Makes failureType required.

@benjben benjben marked this pull request as ready for review June 11, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants