Skip to content
This repository has been archived by the owner on Sep 18, 2021. It is now read-only.

Added handlers to ParrotTransport #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lchristopherson
Copy link

One called upon request with the request data and one called upon response with the request and response data. This allows for more comprehensive monitoring, namely verifying the response time of a request (unless this can already be done in a simpler way).

…st and one called upon response with request and response.
@WamBamBoozle
Copy link

Thanks for the patch -- before I merge this let me point out that normally we handle responses in the record processor. This patch would be more meaningful if the changes were being used to implement more monitoring or if there were some example of its use.

@maniksurtani
Copy link

The point of this patch is to be able to time requests. For that, a callback with a response after making the call isn't enough, hence the callback with the request before making the call.

Sample implementation:

  val requestMap = ... // Track request start times

  override def handleRequest(request: Req) {
    // Add start time
    requestMap.put(request, Time.now)
  }

  override def handleResponseReq(response: Try[Rep], request: Req) {
    // calculate response time
    requestMap.remove(request)
      .foreach(sent => distribution.addTime((Time.now - sent).inMilliseconds))
  }

We use response times to alter throughput of the load generator.

@WamBamBoozle
Copy link

That is awesome but I wonder if it might be duplicating the metrics already
provided to us by finagle
http://twitter.github.io/finagle/guide/Metrics.html. In particular, I
think the finagle metric request_latency_ms is precisely what you are
describing.

Also, I can't help but wonder at the motivation: to alter the throughput of
the load generator? The whole point of the load generator is to deliver
load regardless of how well the service is handling it. In fact, parrot was
designed to be impervious to how well the victim is holding up. Realism.

On Tue, Jul 14, 2015 at 5:54 PM, Manik Surtani [email protected]
wrote:

The point of this patch is to be able to time requests. For that, a
callback with a response after making the call isn't enough, hence the
callback with the request before making the call.

Sample implementation:

val requestMap = ... // Track request start times

override def handleRequest(request: Req) {
// Add start time
requestMap.put(request, Time.now)
}

override def handleResponseReq(response: Try[Rep], request: Req) {
// calculate response time
requestMap.remove(request)
.foreach(sent => distribution.addTime((Time.now - sent).inMilliseconds))
}

We use response times to alter throughput of the load generator.


Reply to this email directly or view it on GitHub
#45 (comment).

@maniksurtani
Copy link

Thanks for the pointer, looking into extracting this data from Finagle metrics.

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Luke Christopherson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

4 participants