APM-209 #15067
APM-209 #15067
Conversation
arangosh/Benchmark/BenchFeature.cpp
Outdated
| @@ -239,6 +242,24 @@ void BenchFeature::collectOptions(std::shared_ptr<ProgramOptions> options) { | |||
| "--verbose", "print out replies if the HTTP header indicates DB errors", false); | |||
| } | |||
|
|
|||
| void BenchFeature::validateOptions(std::shared_ptr<ProgramOptions> options) { | |||
| if (options->processingResult().touched("--histogram.interval-size")) { | |||
There was a problem hiding this comment.
why do we warn a user even if --histogram.generate is true
There was a problem hiding this comment.
Addressed in latest commit
arangosh/Benchmark/BenchFeature.cpp
Outdated
| pp << std::fixed << std::right << std::setw(12) << std::setprecision(2) | ||
| << percentile << "%"; | ||
| if (_generateHistogram) { | ||
| setupHistogram(pp, builder); |
There was a problem hiding this comment.
it's really hard to follow who is responsible for forming a builder structure.
I'd be in favor of at least moving builder.add("histogram", VPackValue(VPackValueType::Object)); out of setupHistogram, so everything is handled in BenchFeature::start()
There was a problem hiding this comment.
Addressed in latest commit
| @@ -556,31 +605,24 @@ bool BenchFeature::report(ClientFeature& client, std::vector<BenchRunResult> res | |||
| printResult(output, builder); | |||
| builder.close(); | |||
|
|
|||
| std::cout << "Min request time: " << std::setprecision(6) | |||
| std::cout << "Min request time: " << std::setprecision(4) | |||
There was a problem hiding this comment.
is making results less precise intentional? why 6 digits was bad?
There was a problem hiding this comment.
We actually don't need more than 4 digits for handling the measurements in the histogram, it's less than milliseconds
arangosh/Benchmark/BenchFeature.cpp
Outdated
| @@ -482,10 +526,13 @@ void BenchFeature::start() { | |||
| *_result = ret; | |||
| } | |||
|
|
|||
| bool BenchFeature::report(ClientFeature& client, std::vector<BenchRunResult> results, | |||
| void BenchFeature::report(ClientFeature& client, std::vector<BenchRunResult>& results, | |||
There was a problem hiding this comment.
I'd be in favor of passing std::vector<BenchRunResult>& results as std::vector<BenchRunResult> const& results and sort results in a caller. In BenchFeature::report we can add an assertion that results are sorted, like TRI_ASSERT(std::is_sorted(...))
There was a problem hiding this comment.
Addressed in latest commit, but can still have room for more optimization
| @@ -56,119 +56,141 @@ const testPaths = { | |||
| // ////////////////////////////////////////////////////////////////////////////// | |||
|
|
|||
There was a problem hiding this comment.
are there any tests checking new functionality?
There was a problem hiding this comment.
it's because, at the moment, we don't have any infrastructure to test arangobench
Scope & Purpose
This is related to https://arangodb.atlassian.net/browse/APM-209.
Histogram displaying is now switched off by default. For displaying it, the new flag
histogram.generatemust be set to true. Its default value is false for compatibility with other versions and also for complying with the histogram not being displayed by default.If this flag is not set to true, but other histogram flags are addressed, e.g.
--histogram.interval-size 500, everything will still run normally, but a warning message will be displayed saying that the histogram is switched off and setting this flag would not be of use.Backports:
Related Information
Testing & Verification