Skip to content
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

Added new bench target 'bench-fast' #297

Closed
wants to merge 11 commits into from
Closed

Conversation

Player256
Copy link

Resolves #292
Added a bool flag -fast.

Signed-off-by: Player256 <dattucodes@gmail.com>
@@ -101,6 +102,10 @@ func BenchmarkSingleQuery(b *testing.B) {
}

func BenchmarkRangeQuery(b *testing.B) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Signed-off-by: Player256 <dattucodes@gmail.com>
Copy link

@mhoffm-aiven mhoffm-aiven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also do the benchstat?

@Player256
Copy link
Author

yes

Signed-off-by: Player256 <dattucodes@gmail.com>
b
Signed-off-by: Player256 <dattucodes@gmail.com>
Signed-off-by: Player256 <dattucodes@gmail.com>
@mhoffm-aiven
Copy link

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

@Player256
Copy link
Author

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
Copy link

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
Copy link
Collaborator

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
Copy link

mhoffm-aiven commented Aug 2, 2023

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
Copy link
Author

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, 6
samplesPerHour)
defer largeSixHourDataset.Close()
}

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

@Player256
Copy link
Author

@mhoffm-aiven @fpetkovski any thoughts on this?

@MichaHoffmann
Copy link
Contributor

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

nishchay-veer and others added 6 commits August 17, 2023 00:59
* CPU time consumed by each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* 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 <nishchayveer19@gmail.com>

* Generalise timing operator for scalar and aggregate

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* 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 <nishchayveer19@gmail.com>

* 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 <nishchayveer19@gmail.com>

* 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 <nishchayveer19@gmail.com>

* Added analyze function for local testing

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Include ObservableVectorOperator in building the operator tree

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Minor changes for type assertion

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Type casting into model.ObservableVectorOperator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Initialised TimingInformation to avoid nil case

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Next call when Analyze query

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Fixed some checks failing

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Embed struct

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Test code for Query Analyze

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Embed model.OperatorTelemetry

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Assertion function for non-zero CPU Time

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Adding model.OperatorTelemetry to each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Capturing CPUTime of each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* engine: actually execute the query

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* Find time consumed by each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removed unnecessary type casting

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Added analyze function for local testing

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Initialised TimingInformation to avoid nil case

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Next call when Analyze query

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Adding model.OperatorTelemetry to each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Capturing CPUTime of each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Find time consumed by each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removed unnecessary type casting

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Fixed few minor nits

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Added model.OperatorTelemetry to noArgFunOperator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removing type checks for model.TimingInformation

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removed type checks

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removed type checks in TestQueryAnalyze

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Fixed few minors issues

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* modified TestQueryAnalyze to check for child operators

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* linters check passed

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Added operatorTelemetry to each operator

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Changed TimingInformation to TrackedTelemetry for recording other telemetry information

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Remove  nil case in slice of operators

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* removing unnecessary typecasts

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Set name of operators

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removed Explain overheads from AnalyzeOutputNode

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* added setName() method to Analyze

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* fixed engine_test

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Removed name from NoopTelemetry

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>

* Rename CPU Time -> Execution time

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* engine: clean up last CPUTime reference

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* execution: rename more fields

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

---------

Signed-off-by: nishchay-veer <nishchayveer19@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: Nishchay Veer <99465982+nishchay-veer@users.noreply.github.com>
Co-authored-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
* 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 <filip.petkovsky@gmail.com>

* Fix lint

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

* Replace fmt.Errorf with errors.New

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

* Use custom type as key

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

---------

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
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 <filip.petkovsky@gmail.com>
Signed-off-by: Player256 <dattucodes@gmail.com>
Signed-off-by: Player256 <dattucodes@gmail.com>
@Player256
Copy link
Author

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

@Player256 Player256 closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add bench-fast make target which excludes long-running benchmarks
5 participants