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

[3.x] Add hooks for monitoring request speed/active requests #3221

Closed
martinbonnin opened this issue Jul 6, 2021 · 9 comments
Closed

[3.x] Add hooks for monitoring request speed/active requests #3221

martinbonnin opened this issue Jul 6, 2021 · 9 comments

Comments

@martinbonnin
Copy link
Contributor

martinbonnin commented Jul 6, 2021

Follow up from #2674.

@AdamMTGreenberg if you already have an API in mind, feel free to comment here, else we can update as we go.

@AdamMTGreenberg
Copy link
Contributor

No APIs in mind at this very second other than a start/stop for parsing objects, but thank you for opening a dialog.

At the moment ApolloCall.StatusEvent feels a bit too limited in scope. For generic events this is great, but for each, I would love to be able to drill down into details about each piece. Eg, for FETCH_CACHE would love to also be able to get additional info on which cache, start time, stop time, if-failure, and potential cache result information.

This would all be super helpful for performance metrics we want to track.

If you think it would be more helpful, at some point this week I can open a sample diff for what I would have in mind as a great-to-have.

@martinbonnin
Copy link
Contributor Author

Eg, for FETCH_CACHE would love to also be able to get additional info on which cache, start time, stop time, if-failure, and potential cache result information.

We could make each piece of the pipeline report their information in the ExecutionContext. Right now there is response.isFromCache but there could be more. For an example:

class CacheInfo(
  val startMillis: Long,
  val endMillis: Long,
  val hit: Boolean
)

And for network:

class NetworkInfo(
  val startMillis: Long,
  val endMillis: Long,
)

on which cache

Results might be composed from multiple caches but I guess we could take the "deepest" one.

If you think it would be more helpful, at some point this week I can open a sample diff for what I would have in mind as a great-to-have.

Samples are super welcome!

@AdamMTGreenberg
Copy link
Contributor

Created two rough sample PRs from some ideas I was spitballing

@martinbonnin
Copy link
Contributor Author

3.0.0-alpha07 now has:

class HttpInfo(
    val millisStart: Long,
    val millisEnd: Long,
    val statusCode: Int,
    val headers: List<HttpHeader>
)
class CacheInfo(
    val millisStart: Long,
    val millisEnd: Long,
    val hit: Boolean,
    val missedKey: String?,
    val missedField: String?,
)

It also has FlowDecorators that allows more customization:

apolloClient.withFlowDecorator {
    it.onStart {
      // log start
    }.onEach {
      // log response
    }.onCompletion {
      // log end
    }
  }

@AdamMTGreenberg let me know how that looks and if anything else is needed.

@AdamMTGreenberg
Copy link
Contributor

@martinbonnin thanks for this - it looks awesome. I'll have to dig in, but offhand do you know if in HttpInfo the start/stop are triggered when the request is executed or scheduled?

@martinbonnin
Copy link
Contributor Author

when the request is executed or scheduled

You mean what thread the start/stop is measured in? The threading is different on JVM vs Native (see https://github.com/apollographql/apollo-android/blob/dev-3.x/design-docs/Threading.md) but for JVM, the high level scheduling is like:

  • Dispatchers.Main: start the query
  • Dispatchers.IO: record startMillis
  • Dispatchers.IO: synchronously execute the HTTP request
  • Dispatchers.IO: record endMillis
  • Dispatchers.Main: dispatch response

So it will not account for any Thread context switch.

@AdamMTGreenberg
Copy link
Contributor

Thank you!

@martinbonnin
Copy link
Contributor Author

martinbonnin commented Nov 18, 2021

Heads up that flowDecorators are gone with #3538 as they were basically duplicating the Interceptor functionality. We'll add some doc about this. Actually, interceptors now control their Dispatcher so it will allow more fine grained monitoring if needed

@martinbonnin
Copy link
Contributor Author

I'm going to close this issue. There are a bunch of extension points already:

  • NetworkTransport
  • HttpInterceptor
  • ApolloInterceptor
  • HttpRequestComposer

@AdamMTGreenberg let us know if there's something you can't do with those and I'll reopen this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants