search

criteo / criteo-api-python-sdk

4 stars 3 forks source link

Async statistics report status is treated as HTTP error #15

Closed eden881 closed 10 months ago

eden881 commented 10 months ago

Hi 😄

I'm using your SDK for the preview version of the API, because I'm using report dimensions which are only available in async preview reports (such as Country) and they're not available in the latest version of the SDK (2023-10 as of writing this).

I'm able to use AnalyticsApi(client).get_async_adset_report() with a GenerateStatisticsReportRequest model without issues, and I get a report ID back.

However when I try to query the report's status with AnalyticsApi(client).get_async_export_status(report_id), it fails with ApiException because the response from Criteo API is HTTP 303 (see other). It should be treated normally, but the SDK treats it as an exception:

https://github.com/criteo/criteo-api-python-sdk/blob/d34522144f15a02622b73ddae79fb453a7661746/sdks/marketingsolutions_preview/criteo_api_marketingsolutions_preview/rest.py#L216-L229

I'd expect the SDK to not treat status codes below 400 as errors at all, but even if it does it should at least let users override this behaviour, and get_async_export_status() should be set up to use this override if a 303 is expected from the API.

Thanks for your time!

ouvreboite commented 10 months ago

Hi @eden881 thanks for the report. We will look into it shortly.

Regarding https://github.com/criteo/criteo-api-python-sdk/pull/16: this repo (criteo-api-python-sdk) is the result of the automatic SDK generation from https://github.com/criteo/criteo-api-sdk-generator. So any fix will have to be done upstream there, otherwise, they will be overridden every time we regenerate the SDKs

eden881 commented 10 months ago

Hi @eden881 thanks for the report. We will look into it shortly.

Regarding #16: this repo (criteo-api-python-sdk) is the result of the automatic SDK generation from https://github.com/criteo/criteo-api-sdk-generator. So any fix will have to be done upstream there, otherwise, they will be overridden every time we regenerate the SDKs

Thanks for letting me know. I'll close #16 and see if I can contribute something to the API generator.

ouvreboite commented 10 months ago

@eden881 After some internal discussion, we concluded that 303 is not the appropriate status code for this endpoint (currently, it's lacking the Location header, so it cannot be an effective redirection anyway 🙂 ).

Instead, the endpoint will return a 200 (same as when the report is pending). You will be able to use the existing data.attributes.status field in the body to check that the report is ready to be downloaded. This change should be released next week (Monday or Tuesday normally). So you will not need to update the SDK version.

eden881 commented 10 months ago

@ouvreboite Thanks! That's helpful. Closing this for now, but I'd appreciate a ping when the API update is pushed so I can resume my work 😃

P.S: It might be worth looking into the status situation anyway, so users can avoid those types of exceptions in the future. For example, the implementation of aiohttp for this scenario considers anything below 400 as OK.

ouvreboite commented 10 months ago

@eden881 the change has been deployed in production ➡️ the calls to /status for a finished report now return a 200 and should no longer throw an exception.

eden881 commented 10 months ago

@ouvreboite Appreciate it!