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

Android: Allow to provide a different HttpClient implementation #1067

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

anod
Copy link
Member

@anod anod commented Nov 2, 2022

Subset of changes from #1055 to avoid pulling 3rd party library dependencies into SDK

Example of OkHttp implementation which enablex HTTP/2 support

https://github.com/microsoft/cpp_client_telemetry/blob/b6f41f6e9b974c24902b144ebf271e57ec66b2c5/lib/android_build/maesdk-okhttp/src/main/java/com/microsoft/applications/events/okhttp/OkHttpRequest.java

@anod
Copy link
Member Author

anod commented Jan 8, 2023

@lalitb @sid-dahiya I would like to make Android HTTP client replaceable. Any concerns, suggestions?

This will allow us to use our own OkHttp implementation which supports HTTP/2 for AI integration.

@lalitb
Copy link
Contributor

lalitb commented Jan 9, 2023

@anod - I am not sure how much different are APIs for OkHttp and (existing) HttpURLConnection, but do you think we can introduce HTTPClient for OkHttp as a module similar to other HTTP Clients here. So there would be two differnt HTTPClients for Android, and right one would be picked through HTTPClientFactory class as below:

#elif defined(ANRDOID AND WITH_HTTP2)
    std::shared_ptr<IHttpClient> HttpClientFactory::Create() {
        LOG_TRACE("Creating HttpClient_Android");
        return HttpClient_OkHttp_Android::GetClientInstance();
    }
#elif defined(ANDROID)
    std::shared_ptr<IHttpClient> HttpClientFactory::Create() {
        LOG_TRACE("Creating HttpClient_Android");
        return HttpClient_Android::GetClientInstance();
    }

@anod
Copy link
Member Author

anod commented Jan 10, 2023

@anod - I am not sure how much different are APIs for OkHttp and (existing) HttpURLConnection, but do you think we can introduce HTTPClient for OkHttp as a module similar to other HTTP Clients here. So there would be two differnt HTTPClients for Android, and right one would be picked through HTTPClientFactory class as below:

#elif defined(ANRDOID AND WITH_HTTP2)
    std::shared_ptr<IHttpClient> HttpClientFactory::Create() {
        LOG_TRACE("Creating HttpClient_Android");
        return HttpClient_OkHttp_Android::GetClientInstance();
    }
#elif defined(ANDROID)
    std::shared_ptr<IHttpClient> HttpClientFactory::Create() {
        LOG_TRACE("Creating HttpClient_Android");
        return HttpClient_Android::GetClientInstance();
    }

The problem is including okhttp as dependency. This will require client app to use same or similar version of okhttp
You can see the examples in the following article

Usually libraries provide different implementations as a separate libraries, so the client can decide which implementation to use. Example from retrofit:
image

I can followup with adding a maesdk-okhttp module once I test it and confirm the implementation.

@lalitb
Copy link
Contributor

lalitb commented Jan 23, 2023

Sorry for late reply on this. The changes look good in general if we can avoid the intermediate buffer to store the request data.
Tagging @sid-dahiya ,@mkoscumb in they want to look into these changes.

@lalitb
Copy link
Contributor

lalitb commented Jan 31, 2023

@anod - Just wondering if you would like to resolve and go ahead with this PR ?

@anod
Copy link
Member Author

anod commented Feb 1, 2023

@anod - Just wondering if you would like to resolve and go ahead with this PR ?

Yes, sorry missed the reply, I will take a look on the buffer

@anod anod force-pushed the algavris/android-http-refactor branch 2 times, most recently from cccdfe2 to a61567e Compare February 1, 2023 14:25
@anod anod force-pushed the algavris/android-http-refactor branch from a61567e to 6fb1cfd Compare February 1, 2023 14:32
@anod anod requested a review from lalitb February 7, 2023 04:20
Copy link
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes. We can keep for couple of days for @microsoft/1ds-c-sdk-approver to review this before merging.

@anod anod merged commit 236f2da into main Feb 9, 2023
@anod anod deleted the algavris/android-http-refactor branch February 9, 2023 15:23
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.

2 participants