-
Notifications
You must be signed in to change notification settings - Fork 128
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
feat: Multiple Executions for PsPingPlugin #434
base: dev
Are you sure you want to change the base?
Conversation
@AntyaDev , I have 2 additional questions:
|
@deyanp look, Ping is just a plugin that tests physical latency. |
RequestCountFailed: int | ||
RoundtripTimeMin: int64 | ||
RoundtripTimeMax: int64 | ||
RoundtripTimeAvg: float |
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.
Let's simplify it :)
Let's have only RoundtripTime where you will use avg time
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.
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.
yes, but do u understand that if you have > 2 ms you can stop testing
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.
and I don't see a big pint to have min, max here, and ok, fail
it's PING not something critical
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.
Well, I saw the warning you have built in, and was wondering why so. My serverside request duration is between 70 and 100ms in the best case and depending on the load goes up to 300ms per request. Why would I be bothered by 50ms pings? As long as they are stable, I can safely subtract 50ms from the results of my executions, and I am fine ... e.g.:
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.
Can you at least make it possible for having External Pluguns, not part of the NBomber Solution? Currently this seems to be impossibke, due to a dependency on an internal module ...
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.
I meant NBomber.Extensions.InternalExtensions
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.
not sure that I understand the problem.
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.
I cannot currently extract the PsPingPlugin and have outside of the NBomber project due to this dependency ... if I could then I would have my version of PsPingPlugin as I need it, and you can have the cut-down version without min/max/stdev and Executions config.
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.
sure, I will take a look later at it.
if you want send PR
Hosts = hosts |> Array.map Uri | ||
Timeout = 1_000 | ||
} | ||
Executions = 4 |
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.
let's simplify it
let's don't have it configurable but rather use it as a constant value
let's have 3 only invocations not 4
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.
I need to have this configurable, and to be able to increase the value, as sometimes there is a very high variability in the psping values (see my previous reply) ...
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.
Maybe we should run it at the start of test plus and end of the test to capture a bigger diapason?
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.
on the other hand, I think all this brings an accidental complexity.
This is not the primary purpose of NBomber to correctly measure PING
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.
Hi @deyanp
I have created a new project NBomber.Contracts
https://github.com/PragmaticFlow/NBomber/tree/v3/src/NBomber.Contracts
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.
Hi @AntyaDev , not sure what that brings, is it in relation to the discussion about Plugins depending on some InternalExtensions?
It turned out I just need these, so I copy-pasted them in my local project and managed to decouply the PsPingPlugin from the NBomber project:
namespace Framework.NBomber.Extensions
module InternalExtensions =
let inline isNotNull (value) =
not(isNull value)
module Option =
let ofRecord (value: 'T) =
let boxed = box(value)
if isNotNull(boxed) then Some value
else None
and
module internal Framework.NBomber.Domain.Stats.Statistics
open System
module Constants =
let StatsRounding = 2
module Converter =
let inline round (digits: int) (value: float) = Math.Round(value, digits)
I have adjusted a bit the code so that there are also a few "warmup" pings at the beginning, as otherwise I was getting some crazy ping durations ... Still getting such for Azure though, not sure why, even though I am pinging from 1 AKS cluster another AKS cluster both the same vnet and region (the legendary 2ms are not achievable ;). Same setup in GCP is much more stable for some reason ...
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.
The point with NBomber.Contracts is that to develop any plugin; you need to add reference on NBomber.Contracts instead of the complete NBomber projects. In this way, you don't create cycle references. In other words, you fully decoupled.
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.
Regarding 2ms and customization around warmup - from my perspective is a good sign that something went wrong.
But ok, I explained it already several times :)
…p in between, added WarmUpExecutions
ab0f7ed
to
c3a4899
Compare
c5dde49
to
8caddac
Compare
612e45d
to
7454ffd
Compare
4678a91
to
f2fbc64
Compare
9960ae0
to
824134b
Compare
b6fd14d
to
14b6b47
Compare
60b2d02
to
16d6057
Compare
cee3bec
to
7295146
Compare
4ca4c86
to
fc702e8
Compare
@AntyaDev check this out (especially the naming of colums/values in PsPingPluginStatistics)