-
Notifications
You must be signed in to change notification settings - Fork 15
Exclude feeder from service time #10
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?
Conversation
Sorry for the delay I just had a chance to look at this. Do we have any tests for these changes? |
import scala.sys.process._ | ||
|
||
val gatlingVersion = "2.3.0" | ||
val gatlingVersion = "2.3.1" |
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.
Should this be part of a separate issue / PR?
|
||
stmt.onFailure(err => { | ||
val responseTime: ResponseTime = responseTimeBuilder.build() | ||
val responseTime = new ServiceTime(0, 0) |
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.
Would it be clearer to name this serviceTime
instead of responseTime
?
This would also require a change to line 71:
statsEngine.logResponse(session, name, serviceTime.toGatlingResponseTimings, KO, None,
Some(s"$tagString - Preparing: ${err.take(50)}"), List(responseTime.latencyIn(MICROSECONDS), "PRE", logUuid)) Some(s"$tagString - Preparing: ${err.take(50)}")
but to me that seems a bit clearer. We are converting a ServiceTime
to a ResponseTime
. Thoughts?
|
||
stmt.onFailure(err => { | ||
val responseTime = responseTimeBuilder.build() | ||
val responseTime = new ServiceTime(0, 0) |
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.
Same comments as CqlRequestAction
regarding responseTime
vs serviceTime
.
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.
Commented on each item as part of the review. They should be trivial and I'm not bother by pushback leaving this PR as is. It just seems worth discussing those changes first.
This PR makes sure that the feeder generation time is not included in the measured latency when the plugin is configured to measure service time. It also adds a log that confirms which time is being measured for a given scenario.