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

Support EventListener to decorate HttpTransaction recordings #918

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

carlonzo
Copy link

@carlonzo carlonzo commented Nov 1, 2022

📷 Screenshots

image

📄 Context

Added EventListener to decorate recordings for HttoTransaction.

This will allow to record duration for:

  • Call
  • Connect
  • Secure Connect
  • Dns

todo

  • create tests

I am not sure if this feature is interesting for a Chucker user, but wanted to send a PR to start a discussion of it

@carlonzo
Copy link
Author

carlonzo commented Nov 1, 2022

The reason behind this PR is to understand how long a network request actually takes. Today any interceptor added after Chucker that runs expensive operations, could increase the time required to for a request to actually return to the user.

Also will add infos about the time it was required to acquire a connection and the time required by the dns

@cortinico
Copy link
Member

I am not sure if this feature is interesting for a Chucker user, but wanted to send a PR to start a discussion of it

Thanks for sending this PR @carlonzo
I believe we're not looking into adding this degree of granularity around events for Chucker. If you need this type of information (e.g. DNS start/end), you can create your own OkHTTP interceptor and get those information and create your own callbacks. I don't think Chucker should be doing this.

@carlonzo
Copy link
Author

carlonzo commented Nov 6, 2022

Thanks for reviewing my PR.

The reason I created it was because this level of granularity may be interesting to be added to get information regarding the health of the network as well as the the cost of the app network stack. On my own project this was important to gather the cost in duration of the connection as well as the the time spent by all the interceptors added to okhttp.
I believe that adding the EventListener (an interceptor can't give this level of details) as opt-in feature may help developers understand the full picture of every single connection and not just the single request-response data.

Thanks for considering and for maintaining this project.

@cortinico
Copy link
Member

I believe that adding the EventListener (an interceptor can't give this level of details) as opt-in feature may help developers understand the full picture of every single connection and not just the single request-response data.

True but also quite invasive from the API point of view. How do you feel about having a custom interceptor that does this for you? Why do you feel Chucker should do it instead?

@carlonzo
Copy link
Author

carlonzo commented Nov 7, 2022

if you mean okhttp3.Interceptor, it cannot give you the same amount of information that okhttp3.EventListener can give you. And to retrieve metrics around connection duration, dns duration and ssl connection Interceptor is not enough.

I wanted to add this information in Chucker to enhance to overall amount of information a developer could use and see, and think Chucker was the best place to show this.

What part of the API you feel is invasive?

@cortinico
Copy link
Member

What part of the API you feel is invasive?

The fact that you're adding 8 new fields in the database + the fact that you added an internal class ChuckerEventListener inside the .api package. It's also unclear to me how do you expect the users to interact with this.

@carlonzo
Copy link
Author

carlonzo commented Dec 4, 2022

Sorry for the delay, but somehow I didn't see your reply.

Thanks for it. For API here I don't understand what you mean. The public API has changed with a new public method available from the Interceptor where I've added public fun useEventListener(): EventListener.Factory.
The 8 new fields in the DB are not part of the API, but are a fields used by the private implementation. I can easily drop the 8 to 4 if the number if that is an issue. Is the DB supposed to be accessible to the user or external parties?

The ChuckerEventListener in the api package is a mistake. I will move it the internal one.

I can see it my change invasive in 2 points:

  • new api method in the interceptor
  • the HttpTransactionFactory I needed to add the same instance of HttpTransaction per Call, to allow the EventListener to decorate the transaction with the extra infos required.

@cortinico
Copy link
Member

Is the DB supposed to be accessible to the user or external parties?

Nope is not. At least not in the current stage as it's not properly encapsulated.

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

Successfully merging this pull request may close these issues.

2 participants