airbyte icon indicating copy to clipboard operation
airbyte copied to clipboard

🚨 Source Appsflyer: Migrate to V2 Appsflyer API

Open AnjayGoel opened this issue 1 year ago • 10 comments

What

Migrate from obsolete V1 API to V2. Fixes #37403, #35667, #32446.

How

Appsflyer has deprecated & removed the V1 APIs. This PR migrates the connector to the API V2.

Review guide

User Impact

Users will have to replace the API V1 tokens with API V2 tokens as described here.

Can this PR be safely reverted and rolled back?

  • [ ] YES 💚
  • [ ] NO ❌

AnjayGoel avatar May 19 '24 09:05 AnjayGoel

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 Jun 7, 2024 3:20pm

vercel[bot] avatar May 19 '24 09:05 vercel[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 19 '24 09:05 CLAassistant

@AnjayGoel thank you for contributing! Running CI on this now.

I'd like to remove the dockerfile and setup.py while we're here — would you mind rebasing if/when I ping you?

natikgadzhi avatar May 19 '24 22:05 natikgadzhi

@AnjayGoel thank you for contributing! Running CI on this now.

I'd like to remove the dockerfile and setup.py while we're here — would you mind rebasing if/when I ping you?

Sure

AnjayGoel avatar May 20 '24 04:05 AnjayGoel

Time to rebase! #38363 is in.

natikgadzhi avatar May 21 '24 05:05 natikgadzhi

Time to rebase! #38363 is in.

Done

AnjayGoel avatar May 21 '24 07:05 AnjayGoel

Is this connector running for all the streams? I think there are some schema/columns changes on the definition of the output. So maybe it would be nice to check and apply those changes. Here you have a reference about what I am talking about https://github.com/aoelvp94/airbyte/commit/11d984ad817401f3581e0172b86177e4db1c7a06

aoelvp94 avatar May 22 '24 16:05 aoelvp94

Is this connector running for all the streams? I think there are some schema/columns changes on the definition of the output. So maybe it would be nice to check and apply those changes. Here you have a reference about what I am talking about aoelvp94@11d984a

Hi @aoelvp94, thanks for pointing this out. It looks like some streams (geo_report & partners_report) have additional properties, based on the custom events the user sends to AppsFlyer (counter/unique users fields). So, we have to move to a dynamic schema for these streams. I will try to make those changes and update the PR soon

AnjayGoel avatar May 22 '24 20:05 AnjayGoel

Hi @aoelvp94 I've made the necessary changes to move to a dynamic schema. I've also tested all the streams, and everything seems to work fine.

AnjayGoel avatar May 24 '24 17:05 AnjayGoel

Hi @aoelvp94 I've made the necessary changes to move to a dynamic schema. I've also tested all the streams, and everything seems to work fine.

Going to check from my side. I will let you know

aoelvp94 avatar May 24 '24 22:05 aoelvp94

Looks good so far — I'll wait for other feedback, then we can apply formatters and see if we're ready to merge.

One wild question: is AppsFlyer just a REST api under the hood? Should we yolo rewrite this to a low-code connector instead?

Yes, AppsFlyer is a REST API. If we can handle rate limiting & parse CSV responses then I think we should be able to rewrite this with the low-code connector instead.

AnjayGoel avatar May 25 '24 13:05 AnjayGoel

Hi. Which streams are going to be supported by this patch? Because I guess we are just fixing existing streams, not adding new ones by the updated API?

Correct me if I am wrong

arielsnowik avatar May 25 '24 20:05 arielsnowik

Then I have another question. Can the dynamic schema infer data types from csv? Or it just finds the values. I don't understand why if that is the case why the original code have a static schema in the first place

arielsnowik avatar May 25 '24 20:05 arielsnowik

Hi @arielsnowik, The PR fixes the existing streams. I am not entirely sure if some new endpoints were introduced in v2. However, I have noticed that some endpoints, such as reinstalls, the pull API for organic source, and the ad-revenue data API, are missing in the current connector.

Also, The dynamic schema is only needed for the aggregate data APIs due to user-defined events present in the API response. It's not inferring the data types; it's just finding the additional fields and adding them to the schema as numerical type since the aggregate report only contains numbers.

AnjayGoel avatar May 26 '24 19:05 AnjayGoel

Thank you for the response @AnjayGoel

We are analyzing using this patch (or to make our one) for a business project. Might be useful to understand which endpoints will and which ones will not be supported (so we know what to develop if we need them), may we help with that?

About the dynamic schema, so I understand we have a mix between static schema and dynamic schema for different fields, In our version of the patch we were trying to just make all static for simplicity of the solution, I don't know which would be better for long term support, because CSV does not support in a native way dynamic schema I believe (so making it dynamic sounds to me a little over complicated I believe?) correct me if I am wrong in my reasoning.

Let me know what we can do to help you to make this patch complete/ well documented, as I said we are evaluating using/contributing to this patch or just making our own.

arielsnowik avatar May 26 '24 20:05 arielsnowik

Hi @arielsnowik, Following APIs are supported as of now:

Remaining ones are:

Adding support for them should be a simple task.

Also the issue with dynamic schema is because AppsFlyer is aggregating the In-App Events Statistics in the same report. The format of the aggregated report is like this:

Date, Country,Agency/PMD (af_prt),Media Source (pid),Campaign (c),Impressions,Clicks,CTR,Installs,Conversion Rate,Sessions,Loyal Users,Loyal Users/Installs,Total Revenue,Total Cost,ROI,ARPU,Average eCPI,in_app_event_1 (Unique users),in_app_event_1 (Event counter),in_app_event_1 (Sales in USD),in_app_event_2 (Unique users),in_app_event_2 (Event counter),in_app_event_2 (Sales in USD)...

Since everyone will have different In-App Events, its not possible to have a static schema definition. Also the events may be added/removed, changing the report. If we want to keep the schemas static, we can create a different stream for events statistics like this:

Date, Country,Agency/PMD (af_prt),Media Source (pid),Campaign (c), Event Name, Event counter,Sales in USD,Unique users

AnjayGoel avatar May 27 '24 05:05 AnjayGoel

Excellent @AnjayGoel. Thank you for the information about available methods. Would be nice to have that on readme if it is not already

I get it. In app events does not depend on the input, just the user, thus there the need dynamic to make thing generic. (Would have been nice to have read that in appsflyer docs...)

However I believe also your idea for a static schema using additional stream is nice and intuitive, +1. Let me know what we can do to help.

Kind Regards

arielsnowik avatar May 27 '24 13:05 arielsnowik

Hi @arielsnowik, Thanks for the feedback. I will go ahead and add the additional stream, as discussed, soon. I will also document the currently supported endpoints.

AnjayGoel avatar May 27 '24 17:05 AnjayGoel

Hi. @AnjayGoel I have another question. When you refer to aggregated data api Reinstalls (not implemented), I am not finding it on appsflyer docs https://dev.appsflyer.com/hc/reference/overview-11 image

May I be looking wrong at docs? Or maybe it does not exist anymore?

arielsnowik avatar May 27 '24 20:05 arielsnowik

Hi. @AnjayGoel I have another question. When you refer to aggregated data api Reinstalls (not implemented), I am not finding it on appsflyer docs https://dev.appsflyer.com/hc/reference/overview-11 image

May I be looking wrong at docs? Or maybe it does not exist anymore?

My bad, Its in Raw Data API

AnjayGoel avatar May 28 '24 17:05 AnjayGoel

Hi @arielsnowik, I have added the streams and updated the readme.

AnjayGoel avatar Jun 02 '24 11:06 AnjayGoel

Hi @natikgadzhi , @aoelvp94, Please let me know if anything else needs to be done before merging this PR.

AnjayGoel avatar Jun 02 '24 12:06 AnjayGoel

Hi @arielsnowik, I have added the streams and updated the readme.

Nice, thank you!!

As long as the connector is properly tested and there is a clear way (by just reading readme...) for anyone to implement missing endpoints I think it would be ok to merge the PR. Let me know what do you (and the others) think.

Kind Regards

arielsnowik avatar Jun 02 '24 20:06 arielsnowik

lgtm, @natikgadzhi and @marcosmarxm how is it possible to continue with this development?

aoelvp94 avatar Jun 02 '24 20:06 aoelvp94

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (dda0fdee41ff8d4d2abbc2d27b72576d4e6a794f)

natikgadzhi avatar Jun 04 '24 22:06 natikgadzhi

Kicking off CI on this — I am not certain if we have test creds, but if the idea is that the current version is broken, than I just need formatters and static checks to pass.

natikgadzhi avatar Jun 04 '24 22:06 natikgadzhi

@marcosmarxm, this one needs a breaking change migration guide:

source-appsflyer - ❌ Failed - Breaking changes must be accompanied by a migration guide: Migration guide file is missing for source-appsflyer. Please create a migration guide in ./docs/integrations/<connector-type>s/<connector-name>-migrations.md`.
source-appsflyer - ❌ Failed - Breaking change deadline should be a week in the future: The upgrade deadline for the breaking changes in 0.2.0 is less than 7 days from today. Please extend the deadline.
source-appsflyer - ❌ Failed - Connector version in metadata.yaml and pyproject.toml file must match: Version is 0.2.0 in metadata.yaml, but version is 0.1.1 in pyproject.toml. These two files have to be consistent.
Error: 3 checks failed

Can you link breaking change migration guide docs so @AnjayGoel can write one?

@AnjayGoel, this also needs version to be set in both pyproject.toml and metadata.

natikgadzhi avatar Jun 06 '24 01:06 natikgadzhi

@natikgadzhi I have made the changes

AnjayGoel avatar Jun 07 '24 15:06 AnjayGoel

Triggered CI Tests.

marcosmarxm avatar Jun 07 '24 16:06 marcosmarxm

Hello your feedback matters a lot to us. Can you spare just 3 minutes to complete a survey? We're dedicated to making the contributor experience even better, and your input is key to achieving excellence. Thank you for helping us improve!

marcosmarxm avatar Jun 10 '24 14:06 marcosmarxm