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

fix(sdk): sample record validation errors #2794

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

bodinsamuel
Copy link
Collaborator

Describe your changes

Fixes https://linear.app/nango/issue/NAN-1833/reduce-validation-logs-andor-process-in-parallel

  • Sample record validation errors
    It's spamming the infra, making syncs slower than they should be.

  • Send the sample in parallel
    Don't remember why I didn't do it like this. It complexifies the code a bit but faster; since we sample it's no longer a big problem but can be useful if we increase the sampling or remove it.

The wording is not crazy good, if you have suggestions...

Screenshot 2024-10-01 at 11 37 15

@bodinsamuel bodinsamuel self-assigned this Oct 1, 2024
Copy link

linear bot commented Oct 1, 2024

metrics.increment(metrics.Types.RUNNER_INVALID_SYNCS_RECORDS);

if (this.runnerFlags?.validateSyncRecords) {
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this if for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

early break if enabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

otherwise it just logs everything*

Copy link
Collaborator

Choose a reason for hiding this comment

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

hasErrors is never gonna have more than 1 element then if we always early break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes sorry it's maybe not clear, this flag is only set to true in CLI

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok. so in CLI it breaks out after the first validation error

@bodinsamuel bodinsamuel merged commit b62def4 into master Oct 2, 2024
21 checks passed
@bodinsamuel bodinsamuel deleted the fix/sample-records-validation branch October 2, 2024 08:26
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.

3 participants