-
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 4 commits
e19ecf3
e141e7e
ab26f42
ed9af20
4f2e019
e7dd96f
ed91c57
28807a1
02c7ea0
fd321cb
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") | ||
| } | ||
|
buongarzoni marked this conversation as resolved.
Outdated
|
||
|
|
||
| group = "com.rollbar.okhttp" | ||
|
buongarzoni marked this conversation as resolved.
|
||
| version = "2.2.0" | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| 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") | ||
|
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. 🟡 rollbar-okhttp/build.gradle.kts:8 declares Extended reasoning...What the bug is
Why the build still works today Gradle's default conflict resolution strategy for the same module-coordinate with two versions is highest-wins. Both 5.18.0 and 5.23.0 are valid, the API surface used by these tests is stable across both, and so resolution picks 5.23.0 and tests run green. There is no compile or runtime error, and no test failure. That is exactly why the divergence is silent and why this is filed as a maintainability concern rather than a functional bug. Why it still matters Two concrete maintenance hazards:
Why existing code doesn't prevent it There is no version-alignment task or platform/BOM constraint enforcing a single mockito version across the SDK. The root subprojects block adds the dependency by string coordinate, and a local override on the same coordinate stacks rather than shadows; Gradle simply resolves the conflict at configuration time without any warning. Step-by-step proof
How to fix it Either drop line 8 entirely (preferred — matches every other module) so the inherited 5.18.0 is used, or, if 5.23.0 was deliberately needed for OkHttp 5 / mockwebserver 5 compatibility, bump the root version on line 55 of |
||
| implementation("com.squareup.okhttp3:okhttp:5.3.2") | ||
|
buongarzoni marked this conversation as resolved.
|
||
| 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; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
| 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.
🟡 The Installation snippet in rollbar-okhttp/README.md only lists
com.rollbar:rollbar-okhttpandcom.squareup.okhttp3:okhttp, but the recorder example below callsrollbar.recordNetworkEventFor(...)androllbar.log(...)— methods defined oncom.rollbar.notifier.RollbarBasein the rollbar-java module.rollbar-okhttp/build.gradle.ktsonly declaresapi(project(":rollbar-api")), and rollbar-api contains noRollbarnotifier class, so a user who copy-pastes the install snippet alongside the recorder example will get acannot find symbolcompile error forRollbar/recordNetworkEventFor/log. Add a notifier module (e.g.com.rollbar:rollbar-java) to the install snippet, or add a note that one of the rollbar-* notifier modules is required as a prerequisite.Extended reasoning...
What the bug is and how it manifests
The Installation section of
rollbar-okhttp/README.md(lines 14–30) shows two and only two dependencies:Immediately below, the Usage section's recorder example calls
rollbar.recordNetworkEventFor(level, method, url, statusCode)androllbar.log(exception). Both methods live oncom.rollbar.notifier.RollbarBasein the rollbar-java module — not in rollbar-okhttp and not in rollbar-api. A new user who follows the install snippet literally will not have anyRollbarclass on their compile classpath and will seecannot find symbol: class Rollbar(andmethod recordNetworkEventFor/method log) when they try the example.The specific code path
rollbar-okhttp/build.gradle.ktsdeclares its public dependency tree as:api(project(":rollbar-api"))rollbar-apicontains only payload data classes (Level,RollbarThread, etc.) — no notifier class. The notifier withrecordNetworkEventForandlogis inrollbar-java/src/main/java/com/rollbar/notifier/RollbarBase.java. Consumers who pull in onlyrollbar-okhttptherefore inheritrollbar-apitransitively but notrollbar-java, which the example silently assumes.Why existing code doesn't prevent it
The example uses an unannotated, unconstructed
rollbarvariable — there is no import line, noRollbar rollbar = ...initializer, and no callout that the variable comes from a different module. Nothing in the README points the reader to rollbar-java (or rollbar-web/spring/etc.) as a prerequisite, and nothing in the build script of rollbar-okhttp causes a notifier module to be pulled in transitively.Impact
A first-time adopter of the OkHttp integration who lands on this README and copies the install snippet will get a compile error on the example. Severity is small because most realistic adopters of an OkHttp interceptor for Rollbar will already have rollbar-java configured elsewhere in their app (otherwise they'd have nothing to call
recordNetworkEventForon), but for a clean module-specific quickstart it is still a documentation gap.Step-by-step proof
rollbar-okhttp/README.mdand adds only the two listed dependencies to theirbuild.gradle.kts:com.rollbar:rollbar-okhttpandcom.squareup.okhttp3:okhttp.apiin its build script) plus okhttp.rollbar.recordNetworkEventFor(...)androllbar.log(...).grep -r "class Rollbar " rollbar-api→ no match.grep -rn recordNetworkEventFor rollbar-api→ no match.grep -rn "recordNetworkEventFor" rollbar-java/src/main/java/com/rollbar/notifier/RollbarBase.java→ defined at line 102 (andlog(Throwable)is also defined here).javacfails:error: cannot find symbol: class Rollbarand (after manually importing)error: cannot find symbol: method recordNetworkEventFor.How to fix it
Add a third line to both the Kotlin DSL and Groovy install snippets —
implementation("com.rollbar:rollbar-java:<version>")— or add a one-line note above the snippet stating that one of the rollbar-* notifier modules (e.g.rollbar-java,rollbar-web,rollbar-spring-boot-webmvc) must also be on the classpath to obtain theRollbarnotifier used by the example.