EXPLAIN ANALYZE implementation #8947

stuartcarnie · 2017-10-10T19:12:41Z

EXPLAIN ANALYZE SELECT ... will measure a number of counters whilst
executing the query. All data is processed, providing an accurate
account of blocks that were decoded, cursors created for each
expression in a SELECT statement.

The total planning and execution times are measured.

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

stuartcarnie · 2017-10-11T14:32:07Z

@jsternberg I've broken the PR into multiple commits so it is easier for you to see the changes

stuartcarnie · 2017-10-19T16:52:13Z

Example output

jsternberg · 2017-10-19T16:30:53Z

Godeps

@@ -17,8 +17,10 @@ github.com/peterh/liner 88609521dc4b6c858fd4c98b628147da928ce4ac
 github.com/philhofer/fwd 1612a298117663d7bc9a760ae20d383413859798
 github.com/retailnext/hllpp 38a7bb71b483e855d35010808143beaf05b67f9d
 github.com/spaolacci/murmur3 0d12bf811670bf6a1a63828dfbd003eded177fce
+github.com/stretchr/testify 890a5c3458b43e6104ff5da8dfa139d013d77544


Unless there's a compelling reason, I would prefer we didn't add a testing framework. Is this from custom code or is it a dependency on some copied code?

I agree with not using other testing packages. This package is included only to leverage the assertion package testify/assert. It removes a lot of noise from unit tests when asserting for expected behavior / values. If the assertion fails, it will generate a standard t.Error, t.Fail, etc. For more complex types, like collections, diffs will also be produced.

testify/assert contains APIs for simplifying a number of scenarios and uses the standard Go *testing.T type to report errors or failures. Some example

assert.PanicsWithValue – asserts the code panics and returns an expected value. Much simpler than writing defers, etc

Others I like include:

assert.NotNil

assert.NoError

assert.Empty

assert.Equal

This little introduction is helpful. You can also review the docs for the available assertions.

I'm aware of this testing package. But, we don't use it anywhere else and I don't like the idea of including an additional dependency for only one section of code. Introducing this likely means that it will start being used in other locations and the unit tests won't have the same pattern anymore. They don't currently, but adding additional unit test patterns to a code base that already has a bunch seems like too much to me.

Removed in favor of pkg/testing/assert

jsternberg · 2017-10-19T16:37:31Z

tsdb/engine/tsm1/engine.go

@@ -18,11 +18,15 @@ import (
 	"sync/atomic"
 	"time"

+	"strconv"


This should be grouped with the other imports. I've noticed Gogland seems to do this a lot.

jsternberg · 2017-10-19T17:28:00Z

coordinator/shard_mapper_test.go

@@ -74,7 +75,7 @@ func TestLocalShardMapper(t *testing.T) {
 		t.Fatalf("unexpected number of shard mappings: %d", len(m.ShardMap))
 	}

-	if _, err := ic.CreateIterator(measurement, query.IteratorOptions{}); err != nil {
+	if _, err := ic.CreateIterator(context.TODO(), measurement, query.IteratorOptions{}); err != nil {


Can you modify these various context.TODO() calls to what they're supposed to be?

I'll change them to Background

jsternberg · 2017-10-19T17:42:48Z

pkg/tracing/context.go

@@ -0,0 +1,32 @@
+package tracing


Can you add a doc.go file to this package to explain the behavior? I think I've figured it out, but it's a bit confusing and I don't see very many doc comments.

If I'm correctly, you create a Trace at the beginning of the thing you want to trace. You then use the Span to create child spans. All spans are managed by the Trace and each Span only keeps track of its own level. It does not keep track of anything else.

jsternberg

Squash the commits and just one comment. I'm approving in advance.

jsternberg · 2017-10-20T14:01:08Z

LICENSE_OF_DEPENDENCIES.md

@@ -25,3 +25,5 @@
 - github.com/uber-go/zap [MIT LICENSE](https://github.com/uber-go/zap/blob/master/LICENSE.txt)
 - golang.org/x/crypto [BSD LICENSE](https://github.com/golang/crypto/blob/master/LICENSE)
 - jquery 2.1.4 [MIT LICENSE](https://github.com/jquery/jquery/blob/master/LICENSE.txt)
+- github.com/xlab/treeprint [MIT LICENSE](https://github.com/xlab/treeprint/blob/master/LICENSE)
+- github.com/stretchr/testify [MIT LICENSE](https://github.com/stretchr/testify/blob/master/LICENSE)


This license needs to be removed if we aren't using this dependency.

* Introduces EXPLAIN ANALYZE command, which produces a detailed tree of operations used to execute the query. introduce context.Context to APIs metrics package * create groups of named measurements * safe for concurrent access tracing package EXPLAIN ANALYZE implementation for OSS Serialize EXPLAIN ANALYZE traces from remote nodes use context.Background for tests group with other stdlib packages additional documentation and remove unused API use influxdb/pkg/testing/assert remove testify reference

stuartcarnie requested a review from jsternberg October 10, 2017 19:12

ghost assigned stuartcarnie Oct 10, 2017

ghost added the review label Oct 10, 2017

stuartcarnie force-pushed the sgc-explain branch from a4c3a73 to 53833d2 Compare October 11, 2017 14:31

stuartcarnie force-pushed the sgc-explain branch from 53833d2 to f55a785 Compare October 19, 2017 17:24

jsternberg suggested changes Oct 19, 2017

View reviewed changes

stuartcarnie force-pushed the sgc-explain branch from 947dadf to 82340a5 Compare October 19, 2017 18:25

jsternberg approved these changes Oct 20, 2017

View reviewed changes

e-dard added this to the 1.4.0 milestone Oct 20, 2017

stuartcarnie force-pushed the sgc-explain branch from cef90a4 to e931387 Compare October 20, 2017 15:08

stuartcarnie merged commit 618f0d0 into master Oct 20, 2017

ghost removed the review label Oct 20, 2017

stuartcarnie deleted the sgc-explain branch October 20, 2017 15:35

jsternberg mentioned this pull request Jan 30, 2018

query explainer #7340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EXPLAIN ANALYZE implementation #8947

EXPLAIN ANALYZE implementation #8947

stuartcarnie commented Oct 10, 2017

stuartcarnie commented Oct 11, 2017

stuartcarnie commented Oct 19, 2017

jsternberg Oct 19, 2017

stuartcarnie Oct 19, 2017 •

edited

Loading

jsternberg Oct 19, 2017

stuartcarnie Oct 20, 2017

jsternberg Oct 19, 2017

stuartcarnie Oct 19, 2017

jsternberg Oct 19, 2017

stuartcarnie Oct 19, 2017

stuartcarnie Oct 19, 2017

jsternberg Oct 19, 2017

stuartcarnie Oct 20, 2017

jsternberg left a comment

jsternberg Oct 20, 2017

EXPLAIN ANALYZE implementation #8947

EXPLAIN ANALYZE implementation #8947

Conversation

stuartcarnie commented Oct 10, 2017

Required for all non-trivial PRs

stuartcarnie commented Oct 11, 2017

stuartcarnie commented Oct 19, 2017

Choose a reason for hiding this comment

stuartcarnie Oct 19, 2017 • edited Loading

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

jsternberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartcarnie Oct 19, 2017 •

edited

Loading