EVG-5410 Add functions to query execution statistics #1744

syev · 2018-11-05T21:52:58Z

No description provided.

model/stats/db.go

tychoish · 2018-11-06T01:49:01Z

model/stats/db.go

+}
+
+// Returns the sort order specification (1, -1) for the date field corresponding to the Sort value.
+func sortDateOrder(sort Sort) int {


does it make sense to make this a method on Sort?

As this is database specific, I have a preference for leaving it here.

I'm not sure what "database specific" means?

The value returned, 1 or -1, is destined specifically to be the value of the "date" field in the aggregation pipeline's $sort stage.

tychoish · 2018-11-06T01:49:47Z

model/stats/query.go

+)
+
+type GroupBy string
+type Sort string


should we use iotas here? are we using the string form here?

The string values are not used. We could use iotas as well.

does this need to be exported? Do you have a validation method on these types to ensure that they have valid values?

Yes, the types are part of the API. The validation is currently done in the validate() method of the StatsFilter but we might want to move it to method directly on these types.

model/stats/query.go

tychoish · 2018-11-06T01:59:26Z

model/stats/query.go

+		if len(f.Tasks) == 0 {
+			catcher.Add(errors.New("Missing Tasks values"))
+		}
+	}


can't we avoid needing to pass a bool to the validate function but just inferring from the state of the filter?

The bool indicates what the intended use is, I don't think we'd want to infer it from the filter itself. We could include the bool in the filter but we'd have to check that its value matches its usage anyway.

I'm very much in favor of moving the bool out of the function arguments, in whatever way makes the most sense.

I replaced the bool with two validate methods, one for use with tests stats, one for use with task stats.

model/stats/db.go

tychoish · 2018-11-07T18:03:40Z

model/stats/db.go

+		fields = []string{"_id.date", "_id.variant", "_id.task_name", "_id.test_file", "_id.distro"}
+		operators = []string{dateOperator, "$gt", "$gt", "$gt", "$gt"}
+		values = []interface{}{filter.StartAt.Date, filter.StartAt.BuildVariant, filter.StartAt.Task, filter.StartAt.Test, filter.StartAt.Distro}
+	}


Is there anything that validates that we've specified the right number of arguments to each. It seems really easy to make a change here and get very different semantics. Seems like the operators are "whatever the date operator is plus $gt until the end.

I created a struct to combine the field, operator and value, and now pass a array of the struct.

tychoish · 2018-11-07T18:04:10Z

model/stats/db.go

+		fields = []string{"_id.date", "_id.variant", "_id.task_name", "_id.distro"}
+		operators = []string{dateOperator, "$gt", "$gt", "$gt"}
+		values = []interface{}{filter.StartAt.Date, filter.StartAt.BuildVariant, filter.StartAt.Task, filter.StartAt.Distro}
+	}


see above comment.

model/stats/db.go

model/stats/query_test.go

tychoish · 2018-11-07T18:08:15Z

model/stats/query_test.go

+	suite.Suite
+}
+
+func TestStatsQuerySuite(t *testing.T) {


I think this suite is probably a little too large: the test cases are very large, and it might be better to have some state on the suite object and then reset it in a setupTest fixture to make it easier to test, and more importantly debug fixtures if they change.

tychoish · 2018-11-12T16:30:11Z

model/stats/query.go

+	NumTestFailed      int       `bson:"num_test_failed" json:"num_test_failed"`
+	NumSystemFailed    int       `bson:"num_system_failed" json:"num_system_failed"`
+	NumSetupFailed     int       `bson:"num_setup_failed" json:"num_setup_failed"`
+	AvgDurationSuccess float32   `bson:"avg_duration_success" json:"avg_duration_success"`


mongodb doesn't have 32 bit floats, so lets keep this as float64 just to avoid weirdness.

model/stats/query.go

bsamek · 2018-11-12T16:30:06Z

model/stats/db.go

-			"date":      1,
-			"requester": 1,
+		{"$sort": bson.D{
+			{Name: "project", Value: 1},


These should use ProjectKey, etc., as should the rest of this PR.

bsamek · 2018-11-12T16:30:28Z

model/stats/db.go

+// Queries on the precomputed statistics //
+///////////////////////////////////////////
+
+// Creates an aggregation pipeline to query test statistics.


Comments on should be of the form <functionName> ...

bsamek · 2018-11-12T16:44:02Z

model/stats/db.go

+	return id
+}
+
+// Edits a match expression to include that the 'fieldName' field must have its value in 'values'.


bsamek · 2018-11-12T16:44:27Z

model/stats/db.go

+//	}
+//}
+
+func buildMatchArrayExpression(values []string) interface{} {


Should this return a bson.M?

It can also return a string.

bsamek · 2018-11-12T16:45:41Z

model/stats/db.go

+	return buildPaginationOrBranches(fields)
+}
+
+// Creates an aggregation pipeline to query task statistics.


Nit: Comment style for this function and below.

bsamek · 2018-11-12T16:49:24Z

model/stats/query.go

+	return catcher.Resolve()
+}
+
+// Represents search and aggregation parameters when querying the test or task statistics.


Comment style is especially important for public types.

bsamek · 2018-11-12T16:49:55Z

model/stats/query.go

+		BeforeDate:    before,
+		Tests:         tests,
+		Tasks:         tasks,
+		BuildVariants: nil,


No need to explicitly set fields that are the zero value.

This function has been removed.

model/stats/query.go

bsamek · 2018-11-12T16:52:43Z

model/stats/query.go

+
+// Queries the precomputed task statistics using a filter.
+func GetTaskStats(filter *StatsFilter) ([]TaskStats, error) {
+	err := filter.validateForTasks()


Done in the validation method.

bsamek · 2018-11-12T16:53:27Z

scripts/indexes.js

@@ -147,9 +147,13 @@ db.notifications.ensureIndex({ "sent_at": 1 })

 //======hourly_test_stats======//
 db.hourly_test_stats.createIndex({ "_id.date": 1 }, { expireAfterSeconds: 26 * 7 * 24 * 3600 })  // 26 weeks TTL
+db.hourly_test_stats.createIndex({ "_id.project": 1 }, { "_id.requester": 1}, {"_id.task_name": 1}, {"_id.date": 1})


Is there a ticket open for these indexes?

Yes, they are in the existing ticket.

tychoish · 2018-11-13T16:18:14Z

model/stats/query.go

+
+type GroupBy string
+
+func (gb *GroupBy) validate() error {


I think it's not correct that these are pointer-recievers. Given that they're strings, I think we just remove some * characteers and this all gets better.

tychoish · 2018-11-13T16:18:23Z

model/stats/query.go

+	case GroupByTask:
+	case GroupByTest:
+	default:
+		return errors.Errorf("Invalid GroupBy value: %v", *gb)


tychoish · 2018-11-13T16:20:04Z

model/stats/query.go

+func GetTaskStats(filter *StatsFilter) ([]TaskStats, error) {
+	err := filter.validateForTasks()
+	if err != nil {
+		return nil, err


why not wrap this error?

Rebase on top of master (fa5b44e). Split tests into smaller units. Use struct for pagination fields. Update validation methods. Avoid mutable parameter in match stage construction.

Remove NewDefaultStatsFilter constructor Use float64 instead of float32 Remove json tags Remove unused code Update comments to match conventions Check nil in validateCommon methods

syev requested a review from a team November 5, 2018 21:52

tychoish suggested changes Nov 6, 2018

View reviewed changes

syev force-pushed the evg5410 branch from 2b21079 to 6992160 Compare November 6, 2018 17:24

tychoish suggested changes Nov 7, 2018

View reviewed changes

syev force-pushed the evg5410 branch from 308c41f to ebe1c33 Compare November 8, 2018 20:06

tychoish suggested changes Nov 12, 2018

View reviewed changes

bsamek requested changes Nov 12, 2018

View reviewed changes

bsamek approved these changes Nov 13, 2018

View reviewed changes

tychoish suggested changes Nov 13, 2018

View reviewed changes

tychoish approved these changes Nov 13, 2018

View reviewed changes

Yves Duhem added 9 commits November 13, 2018 17:18

EVG-5410 Add functions to query execution statistics

9ba73b8

PR Feedback

0b10fef

Improve variable name

fb51e1f

PR Feedback 2

64e140f

Rebase on top of master (fa5b44e). Split tests into smaller units. Use struct for pagination fields. Update validation methods. Avoid mutable parameter in match stage construction.

Make earliest sort the default

e1ae129

PR Feedback 3

12af065

Remove NewDefaultStatsFilter constructor Use float64 instead of float32 Remove json tags Remove unused code Update comments to match conventions Check nil in validateCommon methods

Use variables in agg

6dace0b

PR Feedback 4

7d1b6fb

Lint

dd018ab

syev force-pushed the evg5410 branch from 39aab85 to dd018ab Compare November 13, 2018 22:29

syev merged commit 004db50 into evergreen-ci:master Nov 13, 2018

syev deleted the evg5410 branch December 12, 2018 15:33

EVG-5410 Add functions to query execution statistics #1744

EVG-5410 Add functions to query execution statistics #1744

Conversation

syev commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment