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.
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.
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.
Same comments as CqlRequestAction regarding responseTime vs serviceTime.
bradfordcp
left a comment
bradfordcp
left a comment
There was a problem hiding this comment.
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.
2 participants
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.