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

Add a retry event in the EventListener #8663

Open
ddas09 opened this issue Jan 29, 2025 · 6 comments · May be fixed by #8666
Open

Add a retry event in the EventListener #8663

ddas09 opened this issue Jan 29, 2025 · 6 comments · May be fixed by #8666

Comments

@ddas09
Copy link

ddas09 commented Jan 29, 2025

Hi Team,

I want to capture timing metrics like connect, SSL, DNS, and request/response times for each request, including retries. However, since OkHttp reuses the same Call object for retries and doesn't always trigger callFailed(), it isn't easy to differentiate retries from the original request.

Observations:

  • callStart() is only triggered once per Call. And the retries may start at connectStart(), requestHeadersStart(), etc - depending upon the state of the original request.
  • callFailed() isn't always called before a retry, making failure tracking tricky.
  • The same Call instance is used across retries.

Question:
I want to track the metrics for each request separately like below -

{
  "attempt": 1,
  "dnsLookupTimeMs": 50,
  "connectTimeMs": 120,
  "sslHandshakeTimeMs": 80,
  // failure occurs during response body read
},
{
// Reused connection, so no new DNS, connect and SSL lookup
// retry request which succeeds
  "attempt": 2, 
  "responseTime": 200,
  "requestTotalTime": 500
}

How can I achieve this?

@yschimke
Copy link
Collaborator

Looking at any one specific request it's quite doable, but as you say there are many ways it can happen.

https://square.github.io/okhttp/features/events/

We should have tests covering most of these.

  @Test
  fun redirectUsingSameConnectionEventSequence() {
    server.enqueue(
      MockResponse.Builder()
        .code(HttpURLConnection.HTTP_MOVED_TEMP)
        .addHeader("Location: /foo")
        .build(),
    )
    server.enqueue(MockResponse())
    val call = client.newCall(Request.Builder().url(server.url("/")).build())
    call.execute()
    assertThat(listener.recordedEventTypes()).containsExactly(
      "CallStart",
      "ProxySelectStart", "ProxySelectEnd", "DnsStart", "DnsEnd",
      "ConnectStart", "ConnectEnd", "ConnectionAcquired", "RequestHeadersStart",
      "RequestHeadersEnd", "ResponseHeadersStart", "ResponseHeadersEnd", "ResponseBodyStart",
      "ResponseBodyEnd", "RequestHeadersStart", "RequestHeadersEnd", "ResponseHeadersStart",
      "ResponseHeadersEnd", "ResponseBodyStart", "ResponseBodyEnd", "ConnectionReleased",
      "CallEnd",
    )
  }
  @Test
  fun redirectUsingNewConnectionEventSequence() {
...
  @Test
  fun applicationInterceptorProceedsMultipleTimes() {

If there are cases we don't have covered with tests, or there are inconsistencies, please let us know.

If it's really critical, you could combine Interceptors with EventListener. using the network interceptor to inject the events you want for each actual attempt.

Sometimes a custom tag in your Request can be useful to add your own metadata, which can be read by interceptors and event listeners through the Call.

@ddas09
Copy link
Author

ddas09 commented Jan 31, 2025

@yschimke, We are trying to build a library that auto-instruments HTTP requests and collects their network metrics. And, I believe using a custom tag will require the users to add them manually during request creation. We don't want the users to do that.

One option we tried is - adding a custom retry-count header in an interceptor and reading it in the EventListener APIs. But again - the user has to hook in this interceptor at their end - which we don't want.

We want to do something at our end (the library code) to handle the requirement. We don't want the users of our library to configure anything.

@yschimke
Copy link
Collaborator

Maybe we are missing a retry event?

@ddas09
Copy link
Author

ddas09 commented Jan 31, 2025

@yschimke, that would be the ideal solution for my use case - having an event callback that gets invoked whenever a request is retried. But not sure how complex/feasible it would be to implement at the EventListener end.

@yschimke
Copy link
Collaborator

In the short term, I think if you consider a strict ordering of events, events that go backwards indicate a retry.

But longer term we should add these missing events which are useful.

@yschimke yschimke changed the title How to track retries in the EventListener callbacks? Add a retry event in the EventListener Jan 31, 2025
@ddas09
Copy link
Author

ddas09 commented Jan 31, 2025

@yschimke, yeah - for now we would maintain some sort of set to maintain the events and detect the retry. Once we have the retry event callback added in the event listener API, we can use that.

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 a pull request may close this issue.

2 participants