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 Amplitude : Updates cursor_field in the Amplitude Events stream to the proper field server_upload_time #24780

Conversation

mariamthiam
Copy link
Contributor

@mariamthiam mariamthiam commented Apr 3, 2023

What

Addressing Issue #16943 - Source amplitude uses wrong field as "cursor_field"
Set cursor to server_upload_time

How

Updates "cursor_field" in the Amplitude Events stream to the proper field server_upload_time

Recommended reading order

  1. x.java
  2. y.python

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user?
This is a fix PR, it should be a minor release.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)

  • Secrets in the connector's spec are annotated with airbyte_secret

  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.

  • Code reviews completed

  • Connector version has been incremented

  • Documentation updated

    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

@mariamthiam mariamthiam force-pushed the fix-amplitude-cursor-issue-on-events branch from 4728c3b to fcbf6d1 Compare April 3, 2023 11:13
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 3, 2023
@mariamthiam
Copy link
Contributor Author

@Nmaxime and @bazarnov , I'm reopening the PR #17268 here!
We need this to be merged to continue using the connector.

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 3, 2023

/test connector=connectors/source-amplitude

🕑 connectors/source-amplitude https://github.com/airbytehq/airbyte/actions/runs/4598027568
❌ connectors/source-amplitude https://github.com/airbytehq/airbyte/actions/runs/4598027568
🐛 https://gradle.com/s/try2qv35pfloe

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestDiscovery::test_backward_compatibility[inputs0] - co...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
============= 1 failed, 38 passed, 1 skipped in 503.42s (0:08:23) ==============

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 3, 2023

@evantahler @marcosmarxm Are you able to check this PR from your side?
As for CAT, we have failed the test for discovery - backward compatibility checks I think we would need to modify the acceptance-test-config to have it passed.

@mariamthiam Did we answer the question here: #17268 (comment)?
In particular, #17268 (comment) this comment.

@mariamthiam
Copy link
Contributor Author

@bazarnov yes I did the changes with stream_state.get(self.cursor_field, stream_state.get("event_time"))

@mariamthiam
Copy link
Contributor Author

@evantahler @marcosmarxm can you please help with the CI errors ?

@evantahler
Copy link
Contributor

Adding some detail to @bazarnov's comment above - the full error is:

E       connector_acceptance_test.utils.backward_compatibility.NonBackwardCompatibleError: BackwardIncompatibilityContext.DISCOVER - The value of 'default_cursor_field' was changed. Diff: Value of root['events'][0] changed from "event_time" to "server_upload_time".

... which is true. This means that users upgrading from the old version of the connector to this version will be re-importing old data for incremental syncs. We've got 2 choices:

  1. This should be a breaking change
  2. This warning should be ignored, and re-importing data is OK in this intance.

I think I'm comfortable with option 2 in this case - Airbyte doesn't guarantee at-once delivery, and this is fixing a bug. @bazarnov, what do you think?

@mariamthiam to tell our CAT test suite to ignore this change, you'll be modifying acceptance-test-config.yml and adding a bypass reason

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 6, 2023

@evantahler thanks for the question,
I'm totally agree with option 2, if this it is ok for the product side of the question and overall customers satisfaction.

@mariamthiam mariamthiam force-pushed the fix-amplitude-cursor-issue-on-events branch from 98297a0 to 7de6ca0 Compare April 11, 2023 14:31
@mariamthiam
Copy link
Contributor Author

@evantahler and @bazarnov I have done it !

@mariamthiam
Copy link
Contributor Author

should be fix by #22362

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants