Added new bench target 'bench-fast' #297

Player256 · 2023-08-01T09:10:57Z

Resolves #292
Added a bool flag -fast.

Signed-off-by: Player256 <[email protected]>

mhoffm-aiven · 2023-08-01T14:40:29Z

engine/bench_test.go

@@ -101,6 +102,10 @@ func BenchmarkSingleQuery(b *testing.B) {
 }

 func BenchmarkRangeQuery(b *testing.B) {


can we just use tesitng.Short() for this?

Signed-off-by: Player256 <[email protected]>

mhoffm-aiven

Can we also do the benchstat?

Player256 · 2023-08-02T06:38:17Z

yes

Signed-off-by: Player256 <[email protected]>

mhoffm-aiven · 2023-08-02T06:48:41Z

Does it actually work though; the big datasets are still constructed before the benchmark, right? Ill give it a go during EU afternoon.

Player256 · 2023-08-02T06:55:00Z

Does it actually work though; the big datasets are still constructed before the benchmark, right? Ill give it a go during EU afternoon.

hey should I put if statements using testing.short() around the larger dataset.

mhoffm-aiven · 2023-08-02T08:07:30Z

Does it actually work though; the big datasets are still constructed before the benchmark, right? Ill give it a go during EU afternoon.

hey should I put if statements using testing.short() around the larger dataset.

that will hide the identifier probably; but i think it can be structured nicer somewhat; We could factor out the slow benchmarks and skip those completely if we have a "Short" flag.

fpetkovski · 2023-08-02T08:17:08Z

Agree, we should avoid creating the large datasets altogether if -short is provided. We could declare them but not initialize them to make compilation pass.

mhoffm-aiven · 2023-08-02T12:23:09Z

We could add b.Run("fast", ...) where we do construct and use 6h dataset and b.Run("slow", ...) where we construct and use the large datasets and that we skip when the "short" flag is set. wdyt?

Player256 · 2023-08-02T13:45:09Z

We could add b.Run("fast", ...) where we do construct and use 6h dataset and b.Run("slow", ...) where we construct and use the large datasets and that we skip when the "short" flag is set. wdyt?

I was thinking like this :
`var largeSixHourDataset promql.Test
if !testing.Short() {
largeSixHourDataset = setupStorage(b, 10000, 10, 6samplesPerHour)
defer largeSixHourDataset.Close()
}

var sevenDaysAndTwoHoursDataset *promql.Test
if !testing.Short() {
	sevenDaysAndTwoHoursDataset = setupStorage(b, 1000, 3, (7*24+2)*samplesPerHour)
	defer sevenDaysAndTwoHoursDataset.Close()
}`

Player256 · 2023-08-03T13:03:43Z

@mhoffm-aiven @fpetkovski any thoughts on this?

MichaHoffmann · 2023-08-13T12:36:25Z

I would prefer if we group them under "b.Run("fast", ...)" and "b.Run("slow", ...)" and only construct the necessary datasets for the given run

* CPU time consumed by each operator Signed-off-by: nishchay-veer <[email protected]> * Time consumed by operator when calling Next the TimingOperator struct wraps another VectorOperator and includes a startTime field to track the start time of each Next method call. Inside the Next method implementation of TimingOperator, it captures the current time using time.Now() and stores it in the startTime field. Then, it calls the Next method of the wrapped operator. Signed-off-by: nishchay-veer <[email protected]> * Generalise timing operator for scalar and aggregate Signed-off-by: nishchay-veer <[email protected]> * Added recordTime and duration field in numberLiteralSelector added a boolean flag recordTime to enable or disable recording of time taken.If it is enabled, then I have added the code to record the time taken to the duration field. Also modified the constantMetric to include a new label called . Signed-off-by: nishchay-veer <[email protected]> * embedding instead of wrapping operator inside of operator The changes are done for capturing observability information for operators in a clean and modular way. By using embedding instead of wrapping, it allows for more granular data capture without creating cross-references between operators. Signed-off-by: nishchay-veer <[email protected]> * Added Analyze method in ExplainableQuery interface The Analyze method returns an AnalyzeOutputNode, which represents the analysis of the query execution. This analysis can include performance metrics, such as CPU time, memory usage, or other relevant statistics. The Analyze method allows for capturing observability information during query execution to assess performance Signed-off-by: nishchay-veer <[email protected]> * Added analyze function for local testing Signed-off-by: nishchay-veer <[email protected]> * Include ObservableVectorOperator in building the operator tree Signed-off-by: nishchay-veer <[email protected]> * Minor changes for type assertion Signed-off-by: nishchay-veer <[email protected]> * Type casting into model.ObservableVectorOperator Signed-off-by: nishchay-veer <[email protected]> * Initialised TimingInformation to avoid nil case Signed-off-by: nishchay-veer <[email protected]> * Next call when Analyze query Signed-off-by: nishchay-veer <[email protected]> * Fixed some checks failing Signed-off-by: nishchay-veer <[email protected]> * Embed struct Signed-off-by: nishchay-veer <[email protected]> * Test code for Query Analyze Signed-off-by: nishchay-veer <[email protected]> * Embed model.OperatorTelemetry Signed-off-by: nishchay-veer <[email protected]> * Assertion function for non-zero CPU Time Signed-off-by: nishchay-veer <[email protected]> * Adding model.OperatorTelemetry to each operator Signed-off-by: nishchay-veer <[email protected]> * Capturing CPUTime of each operator Signed-off-by: nishchay-veer <[email protected]> * engine: actually execute the query Signed-off-by: Giedrius Statkevičius <[email protected]> * Find time consumed by each operator Signed-off-by: nishchay-veer <[email protected]> * Removed unnecessary type casting Signed-off-by: nishchay-veer <[email protected]> * Added analyze function for local testing Signed-off-by: nishchay-veer <[email protected]> * Initialised TimingInformation to avoid nil case Signed-off-by: nishchay-veer <[email protected]> * Next call when Analyze query Signed-off-by: nishchay-veer <[email protected]> * Adding model.OperatorTelemetry to each operator Signed-off-by: nishchay-veer <[email protected]> * Capturing CPUTime of each operator Signed-off-by: nishchay-veer <[email protected]> * Find time consumed by each operator Signed-off-by: nishchay-veer <[email protected]> * Removed unnecessary type casting Signed-off-by: nishchay-veer <[email protected]> * Fixed few minor nits Signed-off-by: nishchay-veer <[email protected]> * Added model.OperatorTelemetry to noArgFunOperator Signed-off-by: nishchay-veer <[email protected]> * Removing type checks for model.TimingInformation Signed-off-by: nishchay-veer <[email protected]> * Removed type checks Signed-off-by: nishchay-veer <[email protected]> * Removed type checks in TestQueryAnalyze Signed-off-by: nishchay-veer <[email protected]> * Fixed few minors issues Signed-off-by: nishchay-veer <[email protected]> * modified TestQueryAnalyze to check for child operators Signed-off-by: nishchay-veer <[email protected]> * linters check passed Signed-off-by: nishchay-veer <[email protected]> * Added operatorTelemetry to each operator Signed-off-by: nishchay-veer <[email protected]> * Changed TimingInformation to TrackedTelemetry for recording other telemetry information Signed-off-by: nishchay-veer <[email protected]> * Remove nil case in slice of operators Signed-off-by: nishchay-veer <[email protected]> * removing unnecessary typecasts Signed-off-by: nishchay-veer <[email protected]> * Set name of operators Signed-off-by: nishchay-veer <[email protected]> * Removed Explain overheads from AnalyzeOutputNode Signed-off-by: nishchay-veer <[email protected]> * added setName() method to Analyze Signed-off-by: nishchay-veer <[email protected]> * fixed engine_test Signed-off-by: nishchay-veer <[email protected]> * Removed name from NoopTelemetry Signed-off-by: nishchay-veer <[email protected]> * Rename CPU Time -> Execution time Signed-off-by: Giedrius Statkevičius <[email protected]> * engine: clean up last CPUTime reference Signed-off-by: Giedrius Statkevičius <[email protected]> * execution: rename more fields Signed-off-by: Giedrius Statkevičius <[email protected]> --------- Signed-off-by: nishchay-veer <[email protected]> Signed-off-by: Giedrius Statkevičius <[email protected]> Signed-off-by: Nishchay Veer <[email protected]> Co-authored-by: Giedrius Statkevičius <[email protected]>

@MichaHoffmann

* Propagate warnings through context The engine does not return warnings from storage which causes problems with partial response detection in Thanos. I initially thought we had to change the interface of each operator, but @MichaHoffmann had a neat idea to propagate warnings through the context since they have no impact on flow control. Signed-off-by: Filip Petkovski <[email protected]> * Fix lint Signed-off-by: Filip Petkovski <[email protected]> * Replace fmt.Errorf with errors.New Signed-off-by: Filip Petkovski <[email protected]> * Use custom type as key Signed-off-by: Filip Petkovski <[email protected]> --------- Signed-off-by: Filip Petkovski <[email protected]>

This is a follow up commit to thanos-io#298 which enables propagating warnings from remote engines into the distributed query context. Signed-off-by: Filip Petkovski <[email protected]>

Signed-off-by: Player256 <[email protected]>

Player256 · 2023-08-16T19:46:47Z

Why is the dco check failing even though I had signed off in the commits?

added new bench target 'bench-fast'

2e7191b

Signed-off-by: Player256 <[email protected]>

mhoffm-aiven reviewed Aug 1, 2023

View reviewed changes

changed -fast to -short

56c8d8b

Signed-off-by: Player256 <[email protected]>

mhoffm-aiven reviewed Aug 2, 2023

View reviewed changes

Player256 added 3 commits August 2, 2023 12:08

benchstat added in bench-fast target

2ac3b00

Signed-off-by: Player256 <[email protected]>

b

a4bd150

Signed-off-by: Player256 <[email protected]>

benchstat added in bench-fast target

4ef001a

Signed-off-by: Player256 <[email protected]>

nishchay-veer and others added 6 commits August 17, 2023 00:59

Propagate warnings from remote engines (thanos-io#300)

48ef19d

This is a follow up commit to thanos-io#298 which enables propagating warnings from remote engines into the distributed query context. Signed-off-by: Filip Petkovski <[email protected]>

added changes to the bench-fast target

755a3d7

dco check

a9d826a

Signed-off-by: Player256 <[email protected]>

changes made

b7285f9

Signed-off-by: Player256 <[email protected]>

Player256 closed this Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new bench target 'bench-fast' #297

Added new bench target 'bench-fast' #297

Player256 commented Aug 1, 2023

mhoffm-aiven Aug 1, 2023

mhoffm-aiven left a comment

Player256 commented Aug 2, 2023

mhoffm-aiven commented Aug 2, 2023

Player256 commented Aug 2, 2023

mhoffm-aiven commented Aug 2, 2023

fpetkovski commented Aug 2, 2023

mhoffm-aiven commented Aug 2, 2023 •

edited

Player256 commented Aug 2, 2023

Player256 commented Aug 3, 2023

MichaHoffmann commented Aug 13, 2023

Player256 commented Aug 16, 2023

		@@ -101,6 +102,10 @@ func BenchmarkSingleQuery(b *testing.B) {
		}

		func BenchmarkRangeQuery(b *testing.B) {

Added new bench target 'bench-fast' #297

Added new bench target 'bench-fast' #297

Conversation

Player256 commented Aug 1, 2023

mhoffm-aiven Aug 1, 2023

Choose a reason for hiding this comment

mhoffm-aiven left a comment

Choose a reason for hiding this comment

Player256 commented Aug 2, 2023

mhoffm-aiven commented Aug 2, 2023

Player256 commented Aug 2, 2023

mhoffm-aiven commented Aug 2, 2023

fpetkovski commented Aug 2, 2023

mhoffm-aiven commented Aug 2, 2023 • edited

Player256 commented Aug 2, 2023

Player256 commented Aug 3, 2023

MichaHoffmann commented Aug 13, 2023

Player256 commented Aug 16, 2023

mhoffm-aiven commented Aug 2, 2023 •

edited