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

🐛 Source Braintre:: Adds timezone to timestamp in braintree connector #43953

Merged

Conversation

AGPapa
Copy link
Contributor

@AGPapa AGPapa commented Aug 13, 2024

What

The incremental sync for the braintree connector misses newly created records, which I believe is a timezone issue. I think that airbyte is sending the "stream interval start time" as a UTC timestamp but Braintree inteprets it in the local timezone for your account. Testing with the braintree API shows this behavior. If I send a timestamp to the Braintree API without a timezone it interprets it in Eastern time, but if I add +00 it will interpret that as a UTC timestamp.

This means that the incremental sync misses records when in a timezone behind UTC (like North America). There wouldn't be missing data for timezones in Europe that are ahead of UTC, since they would pick up more records than they should and de-dup them.

How

To fix this I formatted the timestamp to include the timezone. After this change the Braintree API interprets it correctly.

Review guide

  1. airbyte-integrations/connectors/source-braintree/source_braintree/manifest.yaml

User Impact

Users in timezones behind UTC will no longer drop records during incremental syncs. Users in timezones ahead of UTC should see no changes.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 20, 2024 5:53am

@AGPapa AGPapa temporarily deployed to community-ci-auto August 13, 2024 00:38 — with GitHub Actions Inactive
@AGPapa AGPapa temporarily deployed to community-ci-auto August 13, 2024 00:38 — with GitHub Actions Inactive
@AGPapa
Copy link
Contributor Author

AGPapa commented Aug 13, 2024

For more detail on how Braintree treats timestamps -

This code returns zero rows.
Screenshot 2024-08-12 at 8 37 53 PM

While this code returns data.
Screenshot 2024-08-12 at 8 38 08 PM

The records in my system were created after 2300 UTC, but before 2300 EDT. I believe Braintree is using the timezone that I configured for my sandbox as the default timezone for these timestamps, however it respects the timestamp in the API call if one is present.

Here are the results of syncs on the transaction table before and after these changes:

I ran a full sync, created a transaction in Braintree and then ran an incremental sync. You can see that the incremental sync did not find the new record.
Screenshot 2024-08-12 at 8 40 50 PM

Here I did the same thing (full sync / create transaction / incremental sync) but after the code changes were applied. This picked up the new record.
Screenshot 2024-08-12 at 8 41 01 PM

Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Looks good! State format is the same, change is in sending over to API, so it's backwards-compatible.

@natikgadzhi natikgadzhi temporarily deployed to community-ci-auto August 20, 2024 05:48 — with GitHub Actions Inactive
@natikgadzhi natikgadzhi temporarily deployed to community-ci-auto August 20, 2024 05:48 — with GitHub Actions Inactive
@natikgadzhi natikgadzhi merged commit 2618e29 into airbytehq:master Aug 20, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community connectors/source/braintree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants