-
Notifications
You must be signed in to change notification settings - Fork 370
feat: Allow serverURL configuration at initialization #884
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
base: master
Are you sure you want to change the base?
Conversation
This change introduces the ability to set the `serverURL` and `proxyServerInteractor` via `MixpanelOptions` during SDK initialization. Applying the `serverURL` from `MixpanelOptions` at initialization time ensures that the ad-blocker check correctly uses the specified host (e.g., `api-eu.mixpanel.com`) instead of the default. The previous `setServerURL()` methods on `MixpanelAPI` have been deprecated in favor of this new approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces the ability to configure serverURL and proxyServerInteractor at SDK initialization time via MixpanelOptions, ensuring that the ad-blocker check uses the correct host from the start. The previous runtime setServerURL() methods on MixpanelAPI are deprecated in favor of this initialization-time configuration.
Changes:
- Added
serverURLandproxyServerInteractorconfiguration toMixpanelOptions.Builder - Modified
HttpServiceto accept and use a configurable server host for ad-blocker checks - Deprecated existing
setServerURL()methods onMixpanelAPIin favor of initialization-time configuration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/mixpanel/android/util/HttpService.java | Added mServerHost field and DEFAULT_SERVER_HOST constant; added constructor overload to accept serverHost parameter; modified ad-blocker check to use configured host instead of hardcoded default |
| src/main/java/com/mixpanel/android/mpmetrics/MixpanelOptions.java | Added serverURL and proxyServerInteractor fields with corresponding builder methods and getters; includes comprehensive documentation |
| src/main/java/com/mixpanel/android/mpmetrics/MixpanelAPI.java | Applied serverURL from options during initialization before AnalyticsMessages creation; deprecated existing setServerURL() methods with clear migration guidance |
| src/main/java/com/mixpanel/android/mpmetrics/AnalyticsMessages.java | Added extractHostFromUrl() helper method to extract host from config endpoint; modified getPoster() to pass extracted serverHost to HttpService constructor |
| src/androidTest/java/com/mixpanel/android/mpmetrics/MPConfigTest.java | Reorganized static imports; added tests for serverURL configuration from MixpanelOptions; fixed spacing in helper method; includes placeholder test for proxy interactor |
src/main/java/com/mixpanel/android/mpmetrics/AnalyticsMessages.java
Outdated
Show resolved
Hide resolved
src/androidTest/java/com/mixpanel/android/mpmetrics/MPConfigTest.java
Outdated
Show resolved
Hide resolved
- Move the default server host URL to `MPConstants`. - Update `AnalyticsMessages` and `HttpService` to use the new constant.
This change removes the deprecation from the `setServerURL` methods, allowing for the server URL to be configured at runtime again.
| this.mServerHost = TextUtils.isEmpty(serverHost) ? DEFAULT_SERVER_HOST : serverHost; | ||
| } | ||
|
|
||
| public HttpService( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to create a new constructor.
The class says:
/** An HTTP utility class for internal use in the Mixpanel library. Not thread-safe. */
This class is for internal use so as long as we adjust our usages, we should extend the constructor, not create a new one.
|
|
||
| @Override | ||
| public void checkIsMixpanelBlocked() { | ||
| final String host = mServerHost; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the backupHost? Shouldn't we check this too if the first host fails? Otherwise, we will disable before trying a backup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. will add check for backup host as well.
|
|
||
| // Apply serverURL from options before AnalyticsMessages is created | ||
| // This ensures the ad-blocker check uses the correct host | ||
| if (options.getServerURL() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels weird to ignore options.getProxyServerInteractor() unless a server url were set. If these 2 have to be linked, should the MixpanelOptions builder link them in a single method, instead of splitting. You shouldn't be allowed to configure a ProxyServerInteractor without also setting a server url if we don't allow that internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, we don’t allow the host app to listen to API calls or supply extra headers unless a custom server URL is provided via MPConfig.setServer(url, interactor). That’s why we initially followed the same pattern in MixpanelOptions.
That said, I agree with your point that splitting this into two separate setters is not ideal. If these are meant to be coupled, the API should reflect that. I’ll add a new method to MixpanelOptions that mirrors MPConfig.setServer(url, interactor) so the server URL and ProxyServerInteractor are configured together, and it’s not possible to set one without the other. This should make the behavior clearer and more consistent.
Refactored the `MixpanelOptions.Builder` API for setting a custom server URL and proxy interactor. The separate `proxyServerInteractor()` method has been removed and replaced with a new overloaded `serverURL(String, ProxyServerInteractor)` method. This change makes the API more intuitive by coupling the proxy interactor directly with the server URL it's intended for. Additionally, Javadocs for server URL configuration have been enhanced with clearer explanations and code examples for routing data to regional servers or a custom proxy.
The ad-blocker detection logic in `checkIsMixpanelBlocked` has been updated to also check the configured backup host. The SDK will now only be considered blocked if both the primary and backup hosts are inaccessible. If the primary host is blocked but the backup is available, the SDK will continue to function.
Add unit tests for the `checkIsMixpanelBlocked` method to verify the logic for detecting when Mixpanel hosts are blocked and correctly handling primary/backup host configurations.
Renamed `checkIsMixpanelBlocked` to `checkIsServerBlocked` and related variables to reflect that the functionality checks for any blocked server host, not just Mixpanel-specific ones. This makes the ad blocker detection logic more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
src/main/java/com/mixpanel/android/mpmetrics/AnalyticsMessages.java:187
- When
mHttpServicealready exists (line 184), only the backup host is updated but not the server host. If the server URL is changed at runtime viasetServerURL(), themServerHostin the existingHttpServiceinstance won't be updated, causing ad-blocker checks and network operations to use the old host. Consider also providing a method to update the server host, or recreate the HttpService when the server URL changes.
} else {
// Update backup host in case it changed at runtime
mHttpService.setBackupHost(mConfig.getBackupHost());
}
| private static boolean sIsServerBlocked; | ||
| private static final int MIN_UNAVAILABLE_HTTP_RESPONSE_CODE = | ||
| HttpURLConnection.HTTP_INTERNAL_ERROR; | ||
| private static final int MAX_UNAVAILABLE_HTTP_RESPONSE_CODE = 599; | ||
|
|
||
| // Package-private for testing | ||
| static void resetServerBlockedState() { | ||
| sIsServerBlocked = false; | ||
| } | ||
|
|
||
| // Package-private for testing | ||
| static boolean isServerBlocked() { | ||
| return sIsServerBlocked; |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static variable sIsServerBlocked is shared across all HttpService instances, but different Mixpanel instances can now have different server URLs (e.g., one pointing to EU servers, another to US servers). If one instance's server is blocked while another's is not, this shared state will cause incorrect behavior. Consider making this an instance variable instead of static, or maintaining separate blocked states per host.
| private static boolean sIsServerBlocked; | |
| private static final int MIN_UNAVAILABLE_HTTP_RESPONSE_CODE = | |
| HttpURLConnection.HTTP_INTERNAL_ERROR; | |
| private static final int MAX_UNAVAILABLE_HTTP_RESPONSE_CODE = 599; | |
| // Package-private for testing | |
| static void resetServerBlockedState() { | |
| sIsServerBlocked = false; | |
| } | |
| // Package-private for testing | |
| static boolean isServerBlocked() { | |
| return sIsServerBlocked; | |
| private boolean mIsServerBlocked; | |
| private static final int MIN_UNAVAILABLE_HTTP_RESPONSE_CODE = | |
| HttpURLConnection.HTTP_INTERNAL_ERROR; | |
| private static final int MAX_UNAVAILABLE_HTTP_RESPONSE_CODE = 599; | |
| // Package-private for testing | |
| void resetServerBlockedState() { | |
| mIsServerBlocked = false; | |
| } | |
| // Package-private for testing | |
| boolean isServerBlocked() { | |
| return mIsServerBlocked; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this change. This follows an existing pattern and would need a separate refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahul-mixpanel Take another quick look. I don't see any reason mIsServerBlocked should be static. The only caller is its HttpService isOnline. isOnline is not a static call and is called with a real instance and not a shared singleton.
| if (options.getServerURL() != null) { | ||
| mConfig.setServerURL(options.getServerURL(), options.getProxyServerInteractor()); |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling mConfig.setServerURL() with options.getServerURL(), there's no null check. While the condition on line 197 verifies the serverURL is not null, it doesn't prevent the ProxyServerInteractor from being null when passed to setServerURL. If the underlying setServerURL(String, ProxyServerInteractor) method doesn't handle a null interactor properly, this could cause issues. Consider explicitly passing null for the interactor case or ensure the called method handles null interactors safely.
| if (options.getServerURL() != null) { | |
| mConfig.setServerURL(options.getServerURL(), options.getProxyServerInteractor()); | |
| final String serverURL = options.getServerURL(); | |
| final ProxyServerInteractor proxyServerInteractor = options.getProxyServerInteractor(); | |
| if (serverURL != null) { | |
| if (proxyServerInteractor != null) { | |
| mConfig.setServerURL(serverURL, proxyServerInteractor); | |
| } else { | |
| mConfig.setServerURL(serverURL); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null is ok and expected for proxyServerInteractor. No change needed.
| * .serverURL("https://my-proxy.example.com", new ProxyServerInteractor() { | ||
| * @Override | ||
| * public Map<String, String> getProxyRequestHeaders() { | ||
| * return Map.of("Authorization", "Bearer " + token); |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code example uses Map.of() which is only available in Java 9+. The Android SDK typically supports Java 8 for broader compatibility. Consider using alternative syntax that works with Java 8, such as creating a HashMap or using Collections.singletonMap for a single entry, to avoid compilation errors for users on Java 8.
| * return Map.of("Authorization", "Bearer " + token); | |
| * Map<String, String> headers = new java.util.HashMap<String, String>(); | |
| * headers.put("Authorization", "Bearer " + token); | |
| * return headers; |
| public void checkIsServerBlocked() { | ||
| final String primaryHost = mServerHost; | ||
| final String backupHost = mBackupHost; | ||
| new Thread(() -> { | ||
| try { | ||
| long startTimeNanos = System.nanoTime(); | ||
| InetAddress primaryInet = InetAddress.getByName(primaryHost); | ||
|
|
||
| if (!isHostBlocked(primaryInet)) { | ||
| sIsServerBlocked = false; | ||
| return; | ||
| } | ||
|
|
||
| // Primary is blocked - check backup if configured | ||
| boolean backupBlocked = true; | ||
| String errorMsg = primaryHost + " is blocked"; | ||
|
|
||
| if (!TextUtils.isEmpty(backupHost)) { | ||
| try { | ||
| backupBlocked = isHostBlocked(InetAddress.getByName(backupHost)); | ||
| if (backupBlocked) { | ||
| errorMsg = primaryHost + " and " + backupHost + " are blocked"; | ||
| } | ||
| } catch (Exception e) { | ||
| errorMsg = primaryHost + " is blocked, backup check failed"; | ||
| } | ||
| } | ||
|
|
||
| sIsServerBlocked = backupBlocked; | ||
| if (backupBlocked) { | ||
| MPLog.v(LOGTAG, "AdBlocker is enabled. " + errorMsg); | ||
| onNetworkError(null, primaryHost, primaryInet.getHostAddress(), | ||
| startTimeNanos, -1, -1, new IOException(errorMsg)); | ||
| } else { | ||
| MPLog.v(LOGTAG, "Primary host blocked, but backup host is available."); | ||
| } | ||
| } catch (Exception e) { | ||
| // Primary check failed, don't assume blocked | ||
| } | ||
| }).start(); | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkIsServerBlocked method spawns a new thread on every call, but there's no mechanism to prevent multiple concurrent invocations. If this method is called multiple times rapidly (e.g., during initialization of multiple Mixpanel instances), it could spawn many threads all trying to resolve DNS and update the shared sIsServerBlocked variable simultaneously, leading to race conditions and excessive thread creation. Consider using a single background thread or an executor service, or add a flag to prevent concurrent execution.
| public HttpService( | ||
| boolean shouldGzipRequestPayload, | ||
| MixpanelNetworkErrorListener networkErrorListener, | ||
| String backupHost) { | ||
| String backupHost, | ||
| String serverHost) { |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter order in the HttpService constructor places backupHost before serverHost, which is counterintuitive since the primary server host is more important than the backup. This could lead to confusion when calling the constructor. Consider reordering to serverHost, backupHost for better clarity, or add parameter names to make the intent clear.
| @Before | ||
| public void setUp() { | ||
| mContext = InstrumentationRegistry.getInstrumentation().getContext(); | ||
| HttpService.resetServerBlockedState(); | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests rely on a shared static sIsServerBlocked variable that is reset in setUp() and tearDown(). If the Android test framework runs tests in parallel, these tests could interfere with each other since they all modify the same global state. While the reset in setup/teardown helps with sequential execution, this design is inherently fragile for parallel test execution. This is related to the broader issue of using a static variable instead of instance state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in scope for now, but AndroidX test orchestrator would also help in this case.
| protected RemoteService getPoster() { | ||
| if (mHttpService == null) { | ||
| String serverHost = extractHostFromUrl(mConfig.getEventsEndpoint()); | ||
| mHttpService = | ||
| new HttpService( | ||
| mConfig.shouldGzipRequestPayload(), mNetworkErrorListener, mConfig.getBackupHost()); | ||
| mConfig.shouldGzipRequestPayload(), | ||
| mNetworkErrorListener, | ||
| mConfig.getBackupHost(), | ||
| serverHost); | ||
| } else { | ||
| // Update backup host in case it changed at runtime | ||
| mHttpService.setBackupHost(mConfig.getBackupHost()); | ||
| } | ||
| return mHttpService; | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getPoster() method has a race condition. While mHttpService is marked volatile, the check-then-act pattern (lines 176-183) is not atomic. Multiple threads could pass the null check simultaneously and create multiple HttpService instances. According to the coding guidelines, all public APIs must handle concurrent access safely. Consider using double-checked locking with synchronization or initialize mHttpService in the constructor.
| service.checkIsServerBlocked(); | ||
| Thread.sleep(ASYNC_WAIT_MS); |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Thread.sleep() to wait for async operations is fragile and can lead to flaky tests. If the system is under load, 1000ms may not be enough for DNS resolution to complete, causing intermittent test failures. According to the coding guidelines, tests should use BlockingQueue for async verification with timeouts. Consider using a CountDownLatch or similar synchronization mechanism with a timeout instead of hard-coded sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a nice change. Can set a loop that checks isServerBlocked every 10ms or so, with a countdown that exists early once detected. Then you can increase the timeout to > 1s (maybe 5s) in case of network issues. Could abstract this into a method so you aren't repeating this logic over and over throughout the test.
| @Test | ||
| public void testServerURLFromMixpanelOptions() { | ||
| MixpanelOptions options = new MixpanelOptions.Builder() | ||
| .serverURL("https://api-eu.mixpanel.com") | ||
| .build(); | ||
|
|
||
| MixpanelAPI mixpanel = MixpanelAPI.getInstance( | ||
| InstrumentationRegistry.getInstrumentation().getContext(), | ||
| UUID.randomUUID().toString(), | ||
| true, | ||
| options | ||
| ); | ||
|
|
||
| MPConfig config = mixpanel.getMPConfig(); | ||
| assertEquals("https://api-eu.mixpanel.com/track/?ip=1", config.getEventsEndpoint()); | ||
| assertEquals("https://api-eu.mixpanel.com/engage/?ip=1", config.getPeopleEndpoint()); | ||
| assertEquals("https://api-eu.mixpanel.com/groups/?ip=1", config.getGroupsEndpoint()); | ||
| assertEquals("https://api-eu.mixpanel.com/flags/", config.getFlagsEndpoint()); | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests verify that serverURL configuration works when set via MixpanelOptions, but there's no test coverage for the ProxyServerInteractor functionality. Consider adding tests that verify the ProxyServerInteractor is properly passed through and used when set via MixpanelOptions.Builder.serverURL(String, ProxyServerInteractor).
| public Builder serverURL(String serverURL, ProxyServerInteractor proxyServerInteractor) { | ||
| this.serverURL = serverURL; | ||
| this.proxyServerInteractor = proxyServerInteractor; | ||
| return this; | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serverURL(String, ProxyServerInteractor) method doesn't validate that the serverURL parameter is non-null. If null is passed, it will be stored and could cause issues later when used. According to the coding guidelines' defensive programming principles, add null validation for the serverURL parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahul-mixpanel at minimum here, we should be using @NonNull annotation.
tylerjroach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nullability annotations would be helpful to customers removing static from isServerBlocked.
| public Builder serverURL(String serverURL, ProxyServerInteractor proxyServerInteractor) { | ||
| this.serverURL = serverURL; | ||
| this.proxyServerInteractor = proxyServerInteractor; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahul-mixpanel at minimum here, we should be using @NonNull annotation.
| * @param serverURL The base URL for API requests. | ||
| * @return This Builder instance for chaining. | ||
| */ | ||
| public Builder serverURL(String serverURL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @NonNull annotation
| if (options.getServerURL() != null) { | ||
| mConfig.setServerURL(options.getServerURL(), options.getProxyServerInteractor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null is ok and expected for proxyServerInteractor. No change needed.
| private static boolean sIsServerBlocked; | ||
| private static final int MIN_UNAVAILABLE_HTTP_RESPONSE_CODE = | ||
| HttpURLConnection.HTTP_INTERNAL_ERROR; | ||
| private static final int MAX_UNAVAILABLE_HTTP_RESPONSE_CODE = 599; | ||
|
|
||
| // Package-private for testing | ||
| static void resetServerBlockedState() { | ||
| sIsServerBlocked = false; | ||
| } | ||
|
|
||
| // Package-private for testing | ||
| static boolean isServerBlocked() { | ||
| return sIsServerBlocked; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahul-mixpanel Take another quick look. I don't see any reason mIsServerBlocked should be static. The only caller is its HttpService isOnline. isOnline is not a static call and is called with a real instance and not a shared singleton.
* Remove static mIsServerBlocked and Remove Thread.sleep from tests * Add nullability annotations to new options * simplify * add annotation
This pull request introduces support for configuring the Mixpanel API server URL and proxy server interactor at initialization time via
MixpanelOptions. This ensures correct endpoint usage and ad-blocker detection, especially for EU servers or custom proxies. The changes update the builder pattern, propagate configuration throughout the codebase, and add tests to verify behavior.Server URL and Proxy Configuration
serverURLandproxyServerInteractorfields toMixpanelOptions, with corresponding builder methods (serverURL(String)andproxyServerInteractor(ProxyServerInteractor)) for initialization-time configuration. [1] [2]MixpanelAPIto apply the server URL and proxy interactor fromMixpanelOptionsbefore instantiatingAnalyticsMessages, ensuring correct host is used for ad-blocker checks and endpoint construction.Endpoint and Host Handling
AnalyticsMessagesandHttpServiceto extract and propagate the server host from the configured endpoints, ensuring network operations and ad-blocker checks use the intended host. [1] [2] [3] [4]API Deprecations
setServerURLmethods inMixpanelAPIin favor of usingMixpanelOptions.Builderfor initialization-time configuration, with updated documentation. [1] [2]Testing
MPConfigTest.javato verify endpoint construction and default behavior when using custom or default server URLs viaMixpanelOptions.Minor Cleanups
MPConfigTest.javafor clarity and consistency. [1] [2]