-
Notifications
You must be signed in to change notification settings - Fork 74
[Feat] [SDK-399] Okhttp interceptor #367
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| # Rollbar OkHttp Integration | ||
|
|
||
| This module provides an [OkHttp Interceptor](https://square.github.io/okhttp/features/interceptors/) that automatically captures network telemetry for the Rollbar Java SDK. | ||
|
|
||
| It records: | ||
|
|
||
| - **Network telemetry events** for HTTP responses with status code `>= 400` (client and server errors). | ||
| - **Error events** for connection failures, timeouts, and other I/O exceptions. | ||
|
|
||
| ## Installation | ||
|
|
||
| ### Gradle (Kotlin DSL) | ||
|
|
||
| ```kotlin | ||
| dependencies { | ||
| implementation("com.rollbar:rollbar-okhttp:<version>") | ||
| implementation("com.squareup.okhttp3:okhttp:<okhttp-version>") | ||
| } | ||
| ``` | ||
|
|
||
| ### Gradle (Groovy) | ||
|
|
||
| ```groovy | ||
| dependencies { | ||
| implementation 'com.rollbar:rollbar-okhttp:<version>' | ||
| implementation 'com.squareup.okhttp3:okhttp:<okhttp-version>' | ||
| } | ||
| ``` | ||
|
|
||
| ## Usage | ||
|
|
||
| ### 1. Implement `NetworkTelemetryRecorder` | ||
|
|
||
| ```java | ||
| NetworkTelemetryRecorder recorder = new NetworkTelemetryRecorder() { | ||
| @Override | ||
| public void recordNetworkEvent(Level level, String method, String url, String statusCode) { | ||
| rollbar.recordNetworkEventFor(level, method, url, statusCode); | ||
| } | ||
|
|
||
| @Override | ||
| public void recordErrorEvent(Exception exception) { | ||
| rollbar.log(exception); | ||
| } | ||
| }; | ||
| ``` | ||
|
|
||
| ### 2. Add the interceptor to your OkHttpClient | ||
|
|
||
| ```java | ||
| OkHttpClient client = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(recorder)) | ||
| .build(); | ||
| ``` | ||
|
|
||
| ### 3. Make requests as usual | ||
|
|
||
| ```java | ||
| Request request = new Request.Builder() | ||
| .url("https://api.example.com/data") | ||
| .build(); | ||
|
|
||
| Response response = client.newCall(request).execute(); | ||
| ``` | ||
|
|
||
| The interceptor will automatically record telemetry events to Rollbar without interfering with the request/response flow. | ||
|
|
||
| ## Behavior | ||
|
|
||
| | Scenario | Action | | ||
| |-----------------------------------|---------------------------------------------------------| | ||
| | Recorder is `null` | No telemetry or log is recorded | | ||
| | Response status `< 400` | No telemetry recorded, response returned normally | | ||
| | Response status `>= 400` | Records a network telemetry event with `Level.CRITICAL` | | ||
| | Connection failure / timeout | Records an error event, then rethrows the `IOException` | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| plugins { | ||
| id("java") | ||
| } | ||
|
|
||
| group = "com.rollbar.okhttp" | ||
| version = "2.2.0" | ||
|
|
||
| repositories { | ||
| mavenCentral() | ||
| } | ||
|
|
||
| dependencies { | ||
| testImplementation(platform("org.junit:junit-bom:5.14.3")) | ||
| testImplementation("org.junit.jupiter:junit-jupiter") | ||
| testRuntimeOnly("org.junit.platform:junit-platform-launcher") | ||
| testImplementation("com.squareup.okhttp3:mockwebserver:5.3.2") | ||
| testImplementation("org.mockito:mockito-core:5.23.0") | ||
| implementation("com.squareup.okhttp3:okhttp:5.3.2") | ||
| api(project(":rollbar-api")) | ||
| } | ||
|
|
||
| tasks.test { | ||
| useJUnitPlatform() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package com.rollbar.okhttp; | ||
|
|
||
| import com.rollbar.api.payload.data.Level; | ||
|
|
||
| public interface NetworkTelemetryRecorder { | ||
| void recordNetworkEvent(Level level, String method, String url, String statusCode); | ||
|
|
||
| void recordErrorEvent(Exception exception); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package com.rollbar.okhttp; | ||
|
|
||
| import com.rollbar.api.payload.data.Level; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import okhttp3.Interceptor; | ||
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
|
|
||
| public class RollbarOkHttpInterceptor implements Interceptor { | ||
|
|
||
| private final NetworkTelemetryRecorder recorder; | ||
|
|
||
| public RollbarOkHttpInterceptor(NetworkTelemetryRecorder recorder) { | ||
| this.recorder = recorder; | ||
| } | ||
|
|
||
| @Override | ||
| public Response intercept(Chain chain) throws IOException { | ||
| Request request = chain.request(); | ||
|
|
||
| try { | ||
| Response response = chain.proceed(request); | ||
|
|
||
| if (response.code() >= 400 && recorder != null) { | ||
| recorder.recordNetworkEvent( | ||
| Level.CRITICAL, | ||
| request.method(), | ||
| request.url().toString(), | ||
| String.valueOf(response.code())); | ||
| } | ||
|
|
||
| return response; | ||
|
|
||
| } catch (IOException e) { | ||
| if (recorder != null) { | ||
| recorder.recordErrorEvent(e); | ||
| } | ||
|
|
||
| throw e; | ||
|
Comment on lines
+23
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 If the user-supplied Extended reasoning...Bug 1 — Response body leak (success path, lines 24–34) In Step-by-step proof (Bug 1): Suppose the server returns a 500 and Bug 2 — IOException masked (error path, lines 36–41) In the Step-by-step proof (Bug 2): Suppose a DNS failure causes Why existing code doesn't prevent it: The Fix: Wrap each recorder call in |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| package com.rollbar.okhttp; | ||
|
|
||
| import com.rollbar.api.payload.data.Level; | ||
| import okhttp3.OkHttpClient; | ||
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
| import okhttp3.mockwebserver.MockResponse; | ||
| import okhttp3.mockwebserver.MockWebServer; | ||
| import okhttp3.mockwebserver.SocketPolicy; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| class RollbarOkHttpInterceptorTest { | ||
|
|
||
| private MockWebServer server; | ||
| private NetworkTelemetryRecorder recorder; | ||
| private OkHttpClient client; | ||
|
|
||
| @BeforeEach | ||
| void setUp() throws IOException { | ||
| server = new MockWebServer(); | ||
| server.start(); | ||
|
|
||
| recorder = mock(NetworkTelemetryRecorder.class); | ||
|
|
||
| client = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(recorder)) | ||
| .build(); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() throws IOException { | ||
| server.shutdown(); | ||
| } | ||
|
|
||
| @Test | ||
| void successfulResponse_doesNotRecordEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(200)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/ok")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(200, response.code()); | ||
| verifyNoInteractions(recorder); | ||
| } | ||
|
|
||
| @Test | ||
| void redirectResponse_doesNotRecordEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(301).addHeader("Location", "/other")); | ||
|
|
||
| OkHttpClient noFollowClient = client.newBuilder().followRedirects(false).build(); | ||
| Request request = new Request.Builder().url(server.url("/redirect")).build(); | ||
| Response response = noFollowClient.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(301, response.code()); | ||
| verifyNoInteractions(recorder); | ||
| } | ||
|
|
||
| @Test | ||
| void clientErrorResponse_recordsNetworkEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(404)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/not-found")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(404, response.code()); | ||
| verify(recorder).recordNetworkEvent( | ||
| eq(Level.CRITICAL), eq("GET"), contains("/not-found"), eq("404")); | ||
| verify(recorder, never()).recordErrorEvent(any()); | ||
| } | ||
|
|
||
| @Test | ||
| void serverErrorResponse_recordsNetworkEvent() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/error")).build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(500, response.code()); | ||
| verify(recorder).recordNetworkEvent( | ||
| eq(Level.CRITICAL), eq("GET"), contains("/error"), eq("500")); | ||
| verify(recorder, never()).recordErrorEvent(any()); | ||
| } | ||
|
|
||
| @Test | ||
| void connectionFailure_recordsErrorEvent() { | ||
| server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/fail")).build(); | ||
|
|
||
| assertThrows(IOException.class, () -> client.newCall(request).execute()); | ||
|
|
||
| verify(recorder).recordErrorEvent(any(IOException.class)); | ||
| verify(recorder, never()).recordNetworkEvent(any(), any(), any(), any()); | ||
| } | ||
|
|
||
| @Test | ||
| void postRequest_recordsCorrectMethod() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| Request request = new Request.Builder() | ||
| .url(server.url("/post")) | ||
| .post(okhttp3.RequestBody.create("body", okhttp3.MediaType.parse("text/plain"))) | ||
| .build(); | ||
| Response response = client.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| verify(recorder).recordNetworkEvent(eq(Level.CRITICAL), eq("POST"), any(), eq("500")); | ||
| } | ||
|
|
||
| @Test | ||
| void nullRecorder_errorResponse_doesNotThrowNPE() throws IOException { | ||
| server.enqueue(new MockResponse().setResponseCode(500)); | ||
|
|
||
| OkHttpClient nullRecorderClient = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(null)) | ||
| .build(); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/error")).build(); | ||
| Response response = nullRecorderClient.newCall(request).execute(); | ||
| response.close(); | ||
|
|
||
| assertEquals(500, response.code()); | ||
| } | ||
|
|
||
| @Test | ||
| void nullRecorder_connectionFailure_doesNotThrow() { | ||
| server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); | ||
|
|
||
| OkHttpClient nullRecorderClient = new OkHttpClient.Builder() | ||
| .addInterceptor(new RollbarOkHttpInterceptor(null)) | ||
| .build(); | ||
|
|
||
| Request request = new Request.Builder().url(server.url("/fail")).build(); | ||
|
|
||
| assertThrows(IOException.class, () -> nullRecorderClient.newCall(request).execute()); | ||
| } | ||
| } |
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.
🔴 rollbar-okhttp/build.gradle.kts hardcodes version = "2.2.0" instead of relying on the root project's version management, so future release bumps to VERSION_NAME will not update this module's published version. Remove the local version declaration to let the root allprojects { version = versionName } take effect, keeping rollbar-okhttp in sync with the rest of the SDK.
Extended reasoning...
What the bug is and how it manifests
The new rollbar-okhttp module sets
version = "2.2.0"directly in itsbuild.gradle.kts(line 6). In Gradle, each subproject's build script is evaluated after the root project'sallprojectsblock, which means a localversion = ...assignment in a subproject overrides whatever the root block set. As a result, rollbar-okhttp will always advertise itself as version 2.2.0 in its published Maven artifact regardless of what VERSION_NAME is set to.The specific code path that triggers it
The root
build.gradle.ktsreadsval versionName = rootProject.extra["VERSION_NAME"] as Stringfromgradle.propertiesand then applies it insideallprojects { version = versionName }. rollbar-okhttp's localversion = "2.2.0"in its own build script then replaces that value during subproject configuration. All other submodules (rollbar-java, rollbar-api, rollbar-web, rollbar-reactive-streams, etc.) omit a local version declaration and therefore correctly inherit the root-level value.Why existing code doesn't prevent it
There is no enforcement mechanism (e.g., a build check or version-consistency task) that asserts all published modules share the same version. The hardcoded string happens to equal the current VERSION_NAME (also "2.2.0"), so the problem is invisible today.
What the impact would be
When the project next bumps VERSION_NAME (say to 2.3.0), every other module will publish as 2.3.0, but rollbar-okhttp will still publish as 2.2.0. Consumers who declare a BOM or uniform version constraint for all rollbar artifacts will get a resolution conflict or silently use the wrong (stale) artifact.
How to fix it
Remove line 6 (
version = "2.2.0") fromrollbar-okhttp/build.gradle.kts. The rootallprojectsblock will then set the version correctly for this module just as it does for every other module.Step-by-step proof
gradle.propertiessetsVERSION_NAME=2.2.0.build.gradle.ktsevaluatesallprojects { version = "2.2.0" }→ rollbar-okhttp.version = "2.2.0" ✓rollbar-okhttp/build.gradle.kts; the lineversion = "2.2.0"reassigns the same value (no visible effect right now).VERSION_NAME=2.3.0ingradle.properties.allprojectssets rollbar-okhttp.version = "2.3.0" ✓ — but is immediately overwritten in step 6.rollbar-okhttp/build.gradle.ktsstill hasversion = "2.2.0"; Gradle overwrites to "2.2.0"../gradlew :rollbar-okhttp:publishpublishescom.rollbar:rollbar-okhttp:2.2.0while all peer artifacts publish as 2.3.0.