From a203313143de80ea0c350e5a33881f0064298dd2 Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 14 Nov 2018 22:44:27 -0600 Subject: [PATCH 1/3] move Logger and Stats to their own packages I'd like to add stat tracking to Roaring, which means it has to be able to import the stats package, which means stats has to be a package rather than part of the pilosa package. If stats stops being in pilosa, it still needs a way to import logger, so logger also has to leave the pilosa package. Then everything using them needs to import them and use package selectors on their names. This doesn't actually add the stats support to roaring, it just makes it so there's a way to import the stats code from something in the roaring package. --- api.go | 3 +- cache.go | 27 +++++++++-------- cluster.go | 9 +++--- diagnostics.go | 5 ++-- field.go | 10 ++++--- fragment.go | 12 ++++---- gossip/gossip.go | 5 ++-- holder.go | 12 ++++---- http/handler.go | 7 +++-- index.go | 10 ++++--- logger.go => logger/logger.go | 6 ++-- server.go | 10 ++++--- server/server.go | 14 +++++---- stats.go => stats/stats.go | 12 ++++---- stats_test.go => stats/stats_test.go | 44 +++++++++++++++------------- statsd/statsd.go | 13 ++++---- translate.go | 7 +++-- view.go | 10 ++++--- 18 files changed, 121 insertions(+), 95 deletions(-) rename logger.go => logger/logger.go (91%) rename stats.go => stats/stats.go (96%) rename stats_test.go => stats/stats_test.go (80%) diff --git a/api.go b/api.go index 03b3bbcb0..7a95ce76e 100644 --- a/api.go +++ b/api.go @@ -28,6 +28,7 @@ import ( "github.com/pilosa/pilosa/pql" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -930,7 +931,7 @@ func (api *API) AvailableShardsByIndex(_ context.Context) map[string]*roaring.Bi // StatsWithTags returns an instance of whatever implementation of StatsClient // pilosa is using with the given tags. -func (api *API) StatsWithTags(tags []string) StatsClient { +func (api *API) StatsWithTags(tags []string) stats.StatsClient { if api.holder == nil || api.cluster == nil { return nil } diff --git a/cache.go b/cache.go index df82a5802..40509ab64 100644 --- a/cache.go +++ b/cache.go @@ -23,6 +23,7 @@ import ( "time" "github.com/pilosa/pilosa/lru" + "github.com/pilosa/pilosa/stats" ) const ( @@ -50,14 +51,14 @@ type cache interface { Top() []bitmapPair // SetStats defines the stats client used in the cache. - SetStats(s StatsClient) + SetStats(s stats.StatsClient) } // lruCache represents a least recently used Cache implementation. type lruCache struct { cache *lru.Cache counts map[uint64]uint64 - stats StatsClient + stats stats.StatsClient } // newLRUCache returns a new instance of LRUCache. @@ -65,7 +66,7 @@ func newLRUCache(maxEntries uint32) *lruCache { c := &lruCache{ cache: lru.New(int(maxEntries)), counts: make(map[uint64]uint64), - stats: NopStatsClient, + stats: stats.NopStatsClient, } c.cache.OnEvicted = c.onEvicted return c @@ -122,7 +123,7 @@ func (c *lruCache) Top() []bitmapPair { } // SetStats defines the stats client used in the cache. -func (c *lruCache) SetStats(s StatsClient) { +func (c *lruCache) SetStats(s stats.StatsClient) { c.stats = s } @@ -150,7 +151,7 @@ type rankCache struct { // thresholdValue is the value of the last item in the cache thresholdValue uint64 - stats StatsClient + stats stats.StatsClient } // NewRankCache returns a new instance of RankCache. @@ -159,7 +160,7 @@ func NewRankCache(maxEntries uint32) *rankCache { maxEntries: maxEntries, thresholdBuffer: int(thresholdFactor * float64(maxEntries)), entries: make(map[uint64]uint64), - stats: NopStatsClient, + stats: stats.NopStatsClient, } } @@ -279,7 +280,7 @@ func (c *rankCache) recalculate() { } // SetStats defines the stats client used in the cache. -func (c *rankCache) SetStats(s StatsClient) { +func (c *rankCache) SetStats(s stats.StatsClient) { c.stats = s } @@ -458,12 +459,12 @@ func (s *simpleCache) Add(id uint64, b *Row) { // nopCache represents a no-op Cache implementation. type nopCache struct { - stats StatsClient + stats stats.StatsClient } // Ensure NopCache implements Cache. var globalNopCache cache = nopCache{ - stats: NopStatsClient, + stats: stats.NopStatsClient, } func (c nopCache) Add(uint64, uint64) {} @@ -471,10 +472,10 @@ func (c nopCache) BulkAdd(uint64, uint64) {} func (c nopCache) Get(uint64) uint64 { return 0 } func (c nopCache) IDs() []uint64 { return []uint64{} } -func (c nopCache) Invalidate() {} -func (c nopCache) Len() int { return 0 } -func (c nopCache) Recalculate() {} -func (c nopCache) SetStats(StatsClient) {} +func (c nopCache) Invalidate() {} +func (c nopCache) Len() int { return 0 } +func (c nopCache) Recalculate() {} +func (c nopCache) SetStats(stats.StatsClient) {} func (c nopCache) Top() []bitmapPair { return []bitmapPair{} diff --git a/cluster.go b/cluster.go index 4b1ab9089..e22781a25 100644 --- a/cluster.go +++ b/cluster.go @@ -31,6 +31,7 @@ import ( "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/roaring" "github.com/pkg/errors" uuid "github.com/satori/go.uuid" @@ -216,7 +217,7 @@ type cluster struct { // nolint: maligned wg sync.WaitGroup closing chan struct{} - logger Logger + logger logger.Logger InternalClient InternalClient } @@ -235,7 +236,7 @@ func newCluster() *cluster { InternalClient: newNopInternalClient(), - logger: NopLogger, + logger: logger.NopLogger, } } @@ -1379,7 +1380,7 @@ type resizeJob struct { mu sync.RWMutex state string - Logger Logger + Logger logger.Logger } // newResizeJob returns a new instance of resizeJob. @@ -1411,7 +1412,7 @@ func newResizeJob(existingNodes []*Node, node *Node, action string) *resizeJob { IDs: ids, action: action, result: make(chan string), - Logger: NopLogger, + Logger: logger.NopLogger, } } diff --git a/diagnostics.go b/diagnostics.go index 673fb0c7f..5ed97940a 100644 --- a/diagnostics.go +++ b/diagnostics.go @@ -24,6 +24,7 @@ import ( "sync" "time" + "github.com/pilosa/pilosa/logger" "github.com/pkg/errors" ) @@ -51,7 +52,7 @@ type diagnosticsCollector struct { client *http.Client - Logger Logger + Logger logger.Logger server *Server } @@ -65,7 +66,7 @@ func newDiagnosticsCollector(host string) *diagnosticsCollector { // nolint: unp start: time.Now(), client: &http.Client{Timeout: 10 * time.Second}, metrics: make(map[string]interface{}), - Logger: NopLogger, + Logger: logger.NopLogger, } } diff --git a/field.go b/field.go index bc656fa54..4189e55a4 100644 --- a/field.go +++ b/field.go @@ -28,8 +28,10 @@ import ( "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/pql" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" ) @@ -69,7 +71,7 @@ type Field struct { rowAttrStore AttrStore broadcaster broadcaster - Stats StatsClient + Stats stats.StatsClient // Field options. options FieldOptions @@ -79,7 +81,7 @@ type Field struct { // Shards with data on any node in the cluster, according to this node. remoteAvailableShards *roaring.Bitmap - logger Logger + logger logger.Logger } // FieldOption is a functional option type for pilosa.fieldOptions. @@ -196,13 +198,13 @@ func newField(path, index, name string, opts FieldOption) (*Field, error) { rowAttrStore: nopStore, broadcaster: NopBroadcaster, - Stats: NopStatsClient, + Stats: stats.NopStatsClient, options: applyDefaultOptions(fo), remoteAvailableShards: roaring.NewBitmap(), - logger: NopLogger, + logger: logger.NopLogger, } return f, nil } diff --git a/fragment.go b/fragment.go index 359628ab5..805112039 100644 --- a/fragment.go +++ b/fragment.go @@ -36,8 +36,10 @@ import ( "github.com/cespare/xxhash" "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/pql" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" ) @@ -116,7 +118,7 @@ type fragment struct { MaxOpN int // Logger used for out-of-band log entries. - Logger Logger + Logger logger.Logger // Row attribute storage. // This is set by the parent field unless overridden for testing. @@ -126,7 +128,7 @@ type fragment struct { // existing value (to clear) prior to setting a new value. mutexVector vector - stats StatsClient + stats stats.StatsClient } // newFragment returns a new instance of Fragment. @@ -140,10 +142,10 @@ func newFragment(path, index, field, view string, shard uint64) *fragment { CacheType: DefaultCacheType, CacheSize: DefaultCacheSize, - Logger: NopLogger, + Logger: logger.NopLogger, MaxOpN: defaultFragmentMaxOpN, - stats: NopStatsClient, + stats: stats.NopStatsClient, } } @@ -1718,7 +1720,7 @@ func (f *fragment) Snapshot() error { defer f.mu.Unlock() return f.snapshot() } -func track(start time.Time, message string, stats StatsClient, logger Logger) { +func track(start time.Time, message string, stats stats.StatsClient, logger logger.Logger) { elapsed := time.Since(start) logger.Printf("%s took %s", message, elapsed) stats.Histogram("snapshot", elapsed.Seconds(), 1.0) diff --git a/gossip/gossip.go b/gossip/gossip.go index ecd663ecf..8f19203f5 100644 --- a/gossip/gossip.go +++ b/gossip/gossip.go @@ -29,6 +29,7 @@ import ( "github.com/hashicorp/memberlist" "github.com/pilosa/pilosa" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/roaring" "github.com/pilosa/pilosa/toml" "github.com/pkg/errors" @@ -47,7 +48,7 @@ type memberSet struct { papi *pilosa.API config *config - Logger pilosa.Logger + Logger logger.Logger logger *log.Logger logOutput io.Writer @@ -170,7 +171,7 @@ func NewMemberSet(cfg Config, api *pilosa.API, options ...memberSetOption) (*mem host := api.Node().URI.Host g := &memberSet{ papi: api, - Logger: pilosa.NopLogger, + Logger: logger.NopLogger, } // options diff --git a/holder.go b/holder.go index 0a1af820b..68643915e 100644 --- a/holder.go +++ b/holder.go @@ -27,7 +27,9 @@ import ( "syscall" "time" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" uuid "github.com/satori/go.uuid" ) @@ -66,7 +68,7 @@ type Holder struct { closing chan struct{} // Stats - Stats StatsClient + Stats stats.StatsClient // Data directory path. Path string @@ -74,7 +76,7 @@ type Holder struct { // The interval at which the cached row ids are persisted to disk. cacheFlushInterval time.Duration - Logger Logger + Logger logger.Logger } // NewHolder returns a new instance of Holder. @@ -89,13 +91,13 @@ func NewHolder() *Holder { NewPrimaryTranslateStore: newNopTranslateStore, broadcaster: NopBroadcaster, - Stats: NopStatsClient, + Stats: stats.NopStatsClient, NewAttrStore: newNopAttrStore, cacheFlushInterval: defaultCacheFlushInterval, - Logger: NopLogger, + Logger: logger.NopLogger, } } @@ -605,7 +607,7 @@ type holderSyncer struct { Cluster *cluster // Stats - Stats StatsClient + Stats stats.StatsClient // Signals that the sync should stop. Closing <-chan struct{} diff --git a/http/handler.go b/http/handler.go index 81e31a82b..1ecbd2fb1 100644 --- a/http/handler.go +++ b/http/handler.go @@ -36,6 +36,7 @@ import ( "github.com/gorilla/handlers" "github.com/gorilla/mux" "github.com/pilosa/pilosa" + "github.com/pilosa/pilosa/logger" "github.com/pkg/errors" ) @@ -44,7 +45,7 @@ import ( type Handler struct { Handler http.Handler - logger pilosa.Logger + logger logger.Logger // Keeps the query argument validators for each handler validators map[string]*queryValidationSpec @@ -95,7 +96,7 @@ func OptHandlerAPI(api *pilosa.API) handlerOption { } } -func OptHandlerLogger(logger pilosa.Logger) handlerOption { +func OptHandlerLogger(logger logger.Logger) handlerOption { return func(h *Handler) error { h.logger = logger return nil @@ -121,7 +122,7 @@ func OptHandlerCloseTimeout(d time.Duration) handlerOption { // NewHandler returns a new instance of Handler with a default logger. func NewHandler(opts ...handlerOption) (*Handler, error) { handler := &Handler{ - logger: pilosa.NopLogger, + logger: logger.NopLogger, closeTimeout: time.Second * 30, } handler.Handler = newRouter(handler) diff --git a/index.go b/index.go index 297c02f20..e06ce1732 100644 --- a/index.go +++ b/index.go @@ -25,7 +25,9 @@ import ( "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" ) @@ -49,9 +51,9 @@ type Index struct { columnAttrs AttrStore broadcaster broadcaster - Stats StatsClient + Stats stats.StatsClient - logger Logger + logger logger.Logger } // NewIndex returns a new instance of Index. @@ -70,8 +72,8 @@ func NewIndex(path, name string) (*Index, error) { columnAttrs: nopStore, broadcaster: NopBroadcaster, - Stats: NopStatsClient, - logger: NopLogger, + Stats: stats.NopStatsClient, + logger: logger.NopLogger, trackExistence: true, }, nil } diff --git a/logger.go b/logger/logger.go similarity index 91% rename from logger.go rename to logger/logger.go index 074da8a36..ed5a2dc26 100644 --- a/logger.go +++ b/logger/logger.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package pilosa +package logger import ( "io" @@ -39,7 +39,7 @@ func (n *nopLogger) Printf(format string, v ...interface{}) {} // Debugf is a no-op implementation of the Logger Debugf method. func (n *nopLogger) Debugf(format string, v ...interface{}) {} -// standardLogger is a basic implementation of pilosa.Logger based on log.Logger. +// standardLogger is a basic implementation of Logger based on log.Logger. type standardLogger struct { logger *log.Logger } @@ -60,7 +60,7 @@ func (s *standardLogger) Logger() *log.Logger { return s.logger } -// verboseLogger is an implementation of pilosa.Logger which includes debug messages. +// verboseLogger is an implementation of Logger which includes debug messages. type verboseLogger struct { logger *log.Logger } diff --git a/server.go b/server.go index 5e6f61087..24385c4ea 100644 --- a/server.go +++ b/server.go @@ -27,7 +27,9 @@ import ( "sync" "time" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -58,7 +60,7 @@ type Server struct { // nolint: maligned // External systemInfo SystemInfo gcNotifier GCNotifier - logger Logger + logger logger.Logger nodeID string uri URI @@ -81,7 +83,7 @@ func (s *Server) Holder() *Holder { // ServerOption is a functional option type for pilosa.Server type ServerOption func(s *Server) error -func OptServerLogger(l Logger) ServerOption { +func OptServerLogger(l logger.Logger) ServerOption { return func(s *Server) error { s.logger = l return nil @@ -176,7 +178,7 @@ func OptServerPrimaryTranslateStoreFunc(tf func(interface{}) TranslateStore) Ser } } -func OptServerStatsClient(sc StatsClient) ServerOption { +func OptServerStatsClient(sc stats.StatsClient) ServerOption { return func(s *Server) error { s.holder.Stats = sc return nil @@ -258,7 +260,7 @@ func NewServer(opts ...ServerOption) (*Server, error) { metricInterval: 0, diagnosticInterval: 0, - logger: NopLogger, + logger: logger.NopLogger, } s.executor = newExecutor(optExecutorInternalQueryClient(s.defaultClient)) s.cluster.InternalClient = s.defaultClient diff --git a/server/server.go b/server/server.go index 6cdc43883..1e140d1f0 100644 --- a/server/server.go +++ b/server/server.go @@ -41,12 +41,14 @@ import ( "github.com/pilosa/pilosa/gopsutil" "github.com/pilosa/pilosa/gossip" "github.com/pilosa/pilosa/http" + "github.com/pilosa/pilosa/logger" + "github.com/pilosa/pilosa/stats" "github.com/pilosa/pilosa/statsd" "github.com/pkg/errors" ) type loggerLogger interface { - pilosa.Logger + logger.Logger Logger() *log.Logger } @@ -185,9 +187,9 @@ func (m *Command) setupLogger() error { } if m.Config.Verbose { - m.logger = pilosa.NewVerboseLogger(m.logOutput) + m.logger = logger.NewVerboseLogger(m.logOutput) } else { - m.logger = pilosa.NewStandardLogger(m.logOutput) + m.logger = logger.NewStandardLogger(m.logOutput) } return nil } @@ -375,14 +377,14 @@ func (m *Command) Close() error { } // newStatsClient creates a stats client from the config -func newStatsClient(name string, host string) (pilosa.StatsClient, error) { +func newStatsClient(name string, host string) (stats.StatsClient, error) { switch name { case "expvar": - return pilosa.NewExpvarStatsClient(), nil + return stats.NewExpvarStatsClient(), nil case "statsd": return statsd.NewStatsClient(host) case "nop", "none": - return pilosa.NopStatsClient, nil + return stats.NopStatsClient, nil default: return nil, errors.Errorf("'%v' not a valid stats client, choose from [expvar, statsd, none].", name) } diff --git a/stats.go b/stats/stats.go similarity index 96% rename from stats.go rename to stats/stats.go index 8f23c77aa..169df0d6d 100644 --- a/stats.go +++ b/stats/stats.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package pilosa +package stats import ( "expvar" @@ -20,6 +20,8 @@ import ( "strings" "sync" "time" + + "github.com/pilosa/pilosa/logger" ) // Expvar global expvar map. @@ -52,7 +54,7 @@ type StatsClient interface { Timing(name string, value time.Duration, rate float64) // SetLogger Set the logger output type - SetLogger(logger Logger) + SetLogger(logger logger.Logger) // Starts the service Open() @@ -74,7 +76,7 @@ func (c *nopStatsClient) Gauge(name string, value float64, rate float64) func (c *nopStatsClient) Histogram(name string, value float64, rate float64) {} func (c *nopStatsClient) Set(name string, value string, rate float64) {} func (c *nopStatsClient) Timing(name string, value time.Duration, rate float64) {} -func (c *nopStatsClient) SetLogger(logger Logger) {} +func (c *nopStatsClient) SetLogger(logger logger.Logger) {} func (c *nopStatsClient) Open() {} func (c *nopStatsClient) Close() error { return nil } @@ -149,7 +151,7 @@ func (c *expvarStatsClient) Timing(name string, value time.Duration, rate float6 } // SetLogger has no logger. -func (c *expvarStatsClient) SetLogger(logger Logger) { +func (c *expvarStatsClient) SetLogger(logger logger.Logger) { } // Open no-op. @@ -221,7 +223,7 @@ func (a MultiStatsClient) Timing(name string, value time.Duration, rate float64) } // SetLogger Sets the StatsD logger output type. -func (a MultiStatsClient) SetLogger(logger Logger) { +func (a MultiStatsClient) SetLogger(logger logger.Logger) { for _, c := range a { c.SetLogger(logger) } diff --git a/stats_test.go b/stats/stats_test.go similarity index 80% rename from stats_test.go rename to stats/stats_test.go index 067cfa991..3da83ce70 100644 --- a/stats_test.go +++ b/stats/stats_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package pilosa_test +package stats_test import ( "context" @@ -23,6 +23,8 @@ import ( "github.com/pilosa/pilosa" "github.com/pilosa/pilosa/http" + "github.com/pilosa/pilosa/logger" + "github.com/pilosa/pilosa/stats" "github.com/pilosa/pilosa/test" ) @@ -32,51 +34,51 @@ func TestMultiStatClient_Expvar(t *testing.T) { hldr := test.MustOpenHolder() defer hldr.Close() - c := pilosa.NewExpvarStatsClient() - ms := make(pilosa.MultiStatsClient, 1) + c := stats.NewExpvarStatsClient() + ms := make(stats.MultiStatsClient, 1) ms[0] = c hldr.Stats = ms hldr.SetBit("d", "f", 0, 0) hldr.SetBit("d", "f", 0, 1) - hldr.SetBit("d", "f", 0, ShardWidth) - hldr.SetBit("d", "f", 0, ShardWidth+2) + hldr.SetBit("d", "f", 0, pilosa.ShardWidth) + hldr.SetBit("d", "f", 0, pilosa.ShardWidth+2) hldr.ClearBit("d", "f", 0, 1) - if pilosa.Expvar.String() != `{"index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}}` { - t.Fatalf("unexpected expvar : %s", pilosa.Expvar.String()) + if stats.Expvar.String() != `{"index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}}` { + t.Fatalf("unexpected expvar : %s", stats.Expvar.String()) } hldr.Stats.CountWithCustomTags("cc", 1, 1.0, []string{"foo:bar"}) - if pilosa.Expvar.String() != `{"cc": 1, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}}` { - t.Fatalf("unexpected expvar : %s", pilosa.Expvar.String()) + if stats.Expvar.String() != `{"cc": 1, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}}` { + t.Fatalf("unexpected expvar : %s", stats.Expvar.String()) } // Gauge creates a unique key, subsequent Gauge calls will overwrite hldr.Stats.Gauge("g", 5, 1.0) hldr.Stats.Gauge("g", 8, 1.0) - if pilosa.Expvar.String() != `{"cc": 1, "g": 8, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}}` { - t.Fatalf("unexpected expvar : %s", pilosa.Expvar.String()) + if stats.Expvar.String() != `{"cc": 1, "g": 8, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}}` { + t.Fatalf("unexpected expvar : %s", stats.Expvar.String()) } // Set creates a unique key, subsequent sets will overwrite hldr.Stats.Set("s", "4", 1.0) hldr.Stats.Set("s", "7", 1.0) - if pilosa.Expvar.String() != `{"cc": 1, "g": 8, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}, "s": "7"}` { - t.Fatalf("unexpected expvar : %s", pilosa.Expvar.String()) + if stats.Expvar.String() != `{"cc": 1, "g": 8, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}, "s": "7"}` { + t.Fatalf("unexpected expvar : %s", stats.Expvar.String()) } // Record timing duration and a uniquely Set key/value dur, _ := time.ParseDuration("123us") hldr.Stats.Timing("tt", dur, 1.0) - if pilosa.Expvar.String() != `{"cc": 1, "g": 8, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}, "s": "7", "tt": 123µs}` { - t.Fatalf("unexpected expvar : %s", pilosa.Expvar.String()) + if stats.Expvar.String() != `{"cc": 1, "g": 8, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}, "s": "7", "tt": 123µs}` { + t.Fatalf("unexpected expvar : %s", stats.Expvar.String()) } // Expvar histogram is implemented as a gauge hldr.Stats.Histogram("hh", 3, 1.0) - if pilosa.Expvar.String() != `{"cc": 1, "g": 8, "hh": 3, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}, "s": "7", "tt": 123µs}` { - t.Fatalf("unexpected expvar : %s", pilosa.Expvar.String()) + if stats.Expvar.String() != `{"cc": 1, "g": 8, "hh": 3, "index:d": {"field:f": {"view:standard": {"shard:0": {"clearBit": 1, "rows": 0, "setBit": 2}, "shard:1": {"rows": 0, "setBit": 2}}}}, "s": "7", "tt": 123µs}` { + t.Fatalf("unexpected expvar : %s", stats.Expvar.String()) } // Expvar should ignore earlier set tags from setbit @@ -92,8 +94,8 @@ func TestStatsCount_TopN(t *testing.T) { hldr.SetBit("d", "f", 0, 0) hldr.SetBit("d", "f", 0, 1) - hldr.SetBit("d", "f", 0, ShardWidth) - hldr.SetBit("d", "f", 0, ShardWidth+2) + hldr.SetBit("d", "f", 0, pilosa.ShardWidth) + hldr.SetBit("d", "f", 0, pilosa.ShardWidth+2) // Execute query. called := false @@ -311,11 +313,11 @@ func (s *MockStats) CountWithCustomTags(name string, value int64, rate float64, } func (c *MockStats) Tags() []string { return nil } -func (c *MockStats) WithTags(tags ...string) pilosa.StatsClient { return c } +func (c *MockStats) WithTags(tags ...string) stats.StatsClient { return c } func (c *MockStats) Gauge(name string, value float64, rate float64) {} func (c *MockStats) Histogram(name string, value float64, rate float64) {} func (c *MockStats) Set(name string, value string, rate float64) {} func (c *MockStats) Timing(name string, value time.Duration, rate float64) {} -func (c *MockStats) SetLogger(logger pilosa.Logger) {} +func (c *MockStats) SetLogger(logger logger.Logger) {} func (c *MockStats) Open() {} func (c *MockStats) Close() error { return nil } diff --git a/statsd/statsd.go b/statsd/statsd.go index 7a9ae6c14..eaf8facf1 100644 --- a/statsd/statsd.go +++ b/statsd/statsd.go @@ -19,7 +19,8 @@ import ( "time" "github.com/DataDog/datadog-go/statsd" - "github.com/pilosa/pilosa" + "github.com/pilosa/pilosa/logger" + "github.com/pilosa/pilosa/stats" ) // StatsD protocol wrapper using the DataDog library that added Tags to the StatsD protocol @@ -34,13 +35,13 @@ const ( ) // Ensure client implements interface. -var _ pilosa.StatsClient = &statsClient{} +var _ stats.StatsClient = &statsClient{} // statsClient represents a StatsD implementation of pilosa.statsClient. type statsClient struct { client *statsd.Client tags []string - logger pilosa.Logger + logger logger.Logger } // NewStatsClient returns a new instance of StatsClient. @@ -52,7 +53,7 @@ func NewStatsClient(host string) (*statsClient, error) { return &statsClient{ client: c, - logger: pilosa.NopLogger, + logger: logger.NopLogger, }, nil } @@ -70,7 +71,7 @@ func (c *statsClient) Tags() []string { } // WithTags returns a new client with additional tags appended. -func (c *statsClient) WithTags(tags ...string) pilosa.StatsClient { +func (c *statsClient) WithTags(tags ...string) stats.StatsClient { return &statsClient{ client: c.client, tags: unionStringSlice(c.tags, tags), @@ -122,7 +123,7 @@ func (c *statsClient) Timing(name string, value time.Duration, rate float64) { } // SetLogger sets the logger for client. -func (c *statsClient) SetLogger(logger pilosa.Logger) { +func (c *statsClient) SetLogger(logger logger.Logger) { c.logger = logger } diff --git a/translate.go b/translate.go index 86ed6c914..669e1e323 100644 --- a/translate.go +++ b/translate.go @@ -15,6 +15,7 @@ import ( "time" "github.com/cespare/xxhash" + "github.com/pilosa/pilosa/logger" "github.com/pkg/errors" ) @@ -68,7 +69,7 @@ type TranslateFile struct { Path string mapSize int - logger Logger + logger logger.Logger // If non-nil, data is streamed from a primary and this is a read-only store. PrimaryTranslateStore TranslateStore primaryID string // unique ID used to identify the primary store @@ -89,7 +90,7 @@ func OptTranslateFileMapSize(mapSize int) TranslateFileOption { return nil } } -func OptTranslateFileLogger(l Logger) TranslateFileOption { +func OptTranslateFileLogger(l logger.Logger) TranslateFileOption { return func(s *TranslateFile) error { s.logger = l return nil @@ -116,7 +117,7 @@ func NewTranslateFile(opts ...TranslateFileOption) *TranslateFile { mapSize: defaultMapSize, - logger: NopLogger, + logger: logger.NopLogger, replicationClosing: make(chan struct{}), primaryStoreEvents: make(chan primaryStoreEvent), diff --git a/view.go b/view.go index a0abb0e9c..128e3b828 100644 --- a/view.go +++ b/view.go @@ -22,8 +22,10 @@ import ( "strings" "sync" + "github.com/pilosa/pilosa/logger" "github.com/pilosa/pilosa/pql" "github.com/pilosa/pilosa/roaring" + "github.com/pilosa/pilosa/stats" "github.com/pkg/errors" ) @@ -50,9 +52,9 @@ type view struct { fragments map[uint64]*fragment broadcaster broadcaster - stats StatsClient + stats stats.StatsClient rowAttrStore AttrStore - logger Logger + logger logger.Logger } // newView returns a new instance of View. @@ -70,8 +72,8 @@ func newView(path, index, field, name string, fieldOptions FieldOptions) *view { fragments: make(map[uint64]*fragment), broadcaster: NopBroadcaster, - stats: NopStatsClient, - logger: NopLogger, + stats: stats.NopStatsClient, + logger: logger.NopLogger, } } From 33add4f1e000343b4909c333350037ededdddd19 Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 14 Nov 2018 23:01:48 -0600 Subject: [PATCH 2/3] proof of concept for stats This commit adds some trivial stat-tracking which can be observed at localhost:10101/debug/vars. However, writes to a locking data structure aren't cheap, so the stat-tracking is by default not compiled. To build it, add the build tag `roaringstats`, which will cause the `statsHit` function to actually do something. Otherwise, it's an empty and inlineable function, meaning the compiler throws it away entirely. This would, in principle, let us get additional visibility into edge cases and which code paths are hot. This is not the same thing as profiling for overall performance; the stat counts aren't affected by whether a particular code path is using a large amount of CPU time, just reporting how often it happens at all. --- roaring/containers.go | 2 + roaring/roaring.go | 71 ++++++++++++++++++++++++++++++++++++ roaring/roaring_nop_stats.go | 8 ++++ roaring/roaring_stats.go | 15 ++++++++ 4 files changed, 96 insertions(+) create mode 100644 roaring/roaring_nop_stats.go create mode 100644 roaring/roaring_stats.go diff --git a/roaring/containers.go b/roaring/containers.go index ed745a915..3fe0814cc 100644 --- a/roaring/containers.go +++ b/roaring/containers.go @@ -64,6 +64,7 @@ func (sc *sliceContainers) PutContainerValues(key uint64, containerType byte, n } func (sc *sliceContainers) Remove(key uint64) { + statsHit("sliceContainers/Remove") i := search64(sc.keys, key) if i < 0 { return @@ -73,6 +74,7 @@ func (sc *sliceContainers) Remove(key uint64) { } func (sc *sliceContainers) insertAt(key uint64, c *Container, i int) { + statsHit("sliceContainers/insertAt") sc.keys = append(sc.keys, 0) copy(sc.keys[i+1:], sc.keys[i:]) sc.keys[i] = key diff --git a/roaring/roaring.go b/roaring/roaring.go index ba258526f..3a4050cea 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -1027,6 +1027,7 @@ func (iv interval16) runlen() int32 { // newContainer returns a new instance of container. func NewContainer() *Container { + statsHit("NewContainer") return &Container{containerType: containerArray} } @@ -1194,6 +1195,7 @@ func (c *Container) add(v uint16) (added bool) { func (c *Container) arrayAdd(v uint16) bool { // Optimize appending to the end of an array container. if c.n > 0 && c.n < ArrayMaxSize && c.isArray() && c.array[c.n-1] < v { + statsHit("arrayAdd/append") c.unmap() c.array = append(c.array, v) return true @@ -1207,11 +1209,13 @@ func (c *Container) arrayAdd(v uint16) bool { // Convert to a bitmap container if too many values are in an array container. if c.n >= ArrayMaxSize { + statsHit("arrayAdd/arrayToBitmap") c.arrayToBitmap() return c.bitmapAdd(v) } // Otherwise insert into array. + statsHit("arrayAdd/insert") c.unmap() i = -i - 1 c.array = append(c.array, 0) @@ -1325,6 +1329,7 @@ func (c *Container) countRuns() (r int32) { // amount of space. func (c *Container) optimize() { if c.n == 0 { + statsHit("optimize/empty") return } runs := c.countRuns() @@ -1341,21 +1346,33 @@ func (c *Container) optimize() { // Then convert accordingly. if c.isArray() { if newType == containerBitmap { + statsHit("optimize/arrayToBitmap") c.arrayToBitmap() } else if newType == containerRun { + statsHit("optimize/arrayToRun") c.arrayToRun() + } else { + statsHit("optimize/arrayUnchanged") } } else if c.isBitmap() { if newType == containerArray { + statsHit("optimize/bitmapToArray") c.bitmapToArray() } else if newType == containerRun { + statsHit("optimize/bitmapToRun") c.bitmapToRun() + } else { + statsHit("optimize/bitmapUnchanged") } } else if c.isRun() { if newType == containerBitmap { + statsHit("optimize/runToBitmap") c.runToBitmap() } else if newType == containerArray { + statsHit("optimize/runToArray") c.runToArray() + } else { + statsHit("optimize/runUnchanged") } } } @@ -1425,6 +1442,7 @@ func (c *Container) bitmapRemove(v uint16) bool { // Convert to array if we go below the threshold. if c.n == ArrayMaxSize { + statsHit("bitmapRemove/bitmapToArray") c.bitmapToArray() } return true @@ -1492,6 +1510,7 @@ func (c *Container) runMax() uint16 { // bitmapToArray converts from bitmap format to array format. func (c *Container) bitmapToArray() { + statsHit("bitmapToArray") c.array = make([]uint16, 0, c.n) c.containerType = containerArray @@ -1515,6 +1534,7 @@ func (c *Container) bitmapToArray() { // arrayToBitmap converts from array format to bitmap format. func (c *Container) arrayToBitmap() { + statsHit("arrayToBitmap") c.bitmap = make([]uint64, bitmapN) c.containerType = containerBitmap @@ -1534,6 +1554,7 @@ func (c *Container) arrayToBitmap() { // runToBitmap converts from RLE format to bitmap format. func (c *Container) runToBitmap() { + statsHit("runToBitmap") c.bitmap = make([]uint64, bitmapN) c.containerType = containerBitmap @@ -1557,6 +1578,7 @@ func (c *Container) runToBitmap() { // bitmapToRun converts from bitmap format to RLE format. func (c *Container) bitmapToRun() { + statsHit("bitmapToRun") c.containerType = containerRun // return early if empty if c.n == 0 { @@ -1613,6 +1635,7 @@ func (c *Container) bitmapToRun() { // arrayToRun converts from array format to RLE format. func (c *Container) arrayToRun() { + statsHit("arrayToRun") c.containerType = containerRun // return early if empty if c.n == 0 { @@ -1640,6 +1663,7 @@ func (c *Container) arrayToRun() { // runToArray converts from RLE format to array format. func (c *Container) runToArray() { + statsHit("runToArray") c.containerType = containerArray c.array = make([]uint16, 0, c.n) @@ -1661,16 +1685,20 @@ func (c *Container) runToArray() { // Clone returns a copy of c. func (c *Container) Clone() *Container { + statsHit("Container/Clone") other := &Container{n: c.n, containerType: c.containerType} switch c.containerType { case containerArray: + statsHit("Container/Clone/Array") other.array = make([]uint16, len(c.array)) copy(other.array, c.array) case containerBitmap: + statsHit("Container/Clone/Bitmap") other.bitmap = make([]uint64, len(c.bitmap)) copy(other.bitmap, c.bitmap) case containerRun: + statsHit("Container/Clone/Run") other.runs = make([]interval16, len(c.runs)) copy(other.runs, c.runs) } @@ -1689,6 +1717,7 @@ func (c *Container) WriteTo(w io.Writer) (n int64, err error) { } func (c *Container) arrayWriteTo(w io.Writer) (n int64, err error) { + statsHit("Container/arrayWriteTo") if len(c.array) == 0 { return 0, nil } @@ -1705,12 +1734,14 @@ func (c *Container) arrayWriteTo(w io.Writer) (n int64, err error) { } func (c *Container) bitmapWriteTo(w io.Writer) (n int64, err error) { + statsHit("Container/bitmapWriteTo") // Write sizeof(uint64) * bitmapN bytes. nn, err := w.Write((*[0xFFFFFFF]byte)(unsafe.Pointer(&c.bitmap[0]))[:(8 * bitmapN)]) return int64(nn), err } func (c *Container) runWriteTo(w io.Writer) (n int64, err error) { + statsHit("Container/runWriteTo") if len(c.runs) == 0 { return 0, nil } @@ -1815,6 +1846,7 @@ func flip(a *Container) *Container { // nolint: deadcode } func flipArray(b *Container) *Container { + statsHit("flipArray") // TODO: actually implement this x := b.Clone() x.arrayToBitmap() @@ -1822,6 +1854,7 @@ func flipArray(b *Container) *Container { } func flipBitmap(b *Container) *Container { + statsHit("flipBitmap") other := &Container{bitmap: make([]uint64, bitmapN), containerType: containerBitmap} for i, bitmap := range b.bitmap { @@ -1833,6 +1866,7 @@ func flipBitmap(b *Container) *Container { } func flipRun(b *Container) *Container { + statsHit("flipRun") // TODO: actually implement this x := b.Clone() x.runToBitmap() @@ -1868,6 +1902,7 @@ func intersectionCount(a, b *Container) int32 { } func intersectionCountArrayArray(a, b *Container) (n int32) { + statsHit("intersectionCount/ArrayArray") na, nb := len(a.array), len(b.array) for i, j := 0, 0; i < na && j < nb; { va, vb := a.array[i], b.array[j] @@ -1884,6 +1919,7 @@ func intersectionCountArrayArray(a, b *Container) (n int32) { } func intersectionCountArrayRun(a, b *Container) (n int32) { + statsHit("intersectionCount/ArrayRun") na, nb := len(a.array), len(b.runs) for i, j := 0, 0; i < na && j < nb; { va, vb := a.array[i], b.runs[j] @@ -1900,6 +1936,7 @@ func intersectionCountArrayRun(a, b *Container) (n int32) { } func intersectionCountRunRun(a, b *Container) (n int32) { + statsHit("intersectionCount/RunRun") na, nb := len(a.runs), len(b.runs) for i, j := 0, 0; i < na && j < nb; { va, vb := a.runs[i], b.runs[j] @@ -1931,6 +1968,7 @@ func intersectionCountRunRun(a, b *Container) (n int32) { } func intersectionCountBitmapRun(a, b *Container) (n int32) { + statsHit("intersectionCount/BitmapRun") for _, iv := range b.runs { n += a.bitmapCountRange(int32(iv.start), int32(iv.last)+1) } @@ -1938,6 +1976,7 @@ func intersectionCountBitmapRun(a, b *Container) (n int32) { } func intersectionCountArrayBitmap(a, b *Container) (n int32) { + statsHit("intersectionCount/ArrayBitmap") ln := len(b.bitmap) for _, val := range a.array { i := int(val >> 6) @@ -1951,6 +1990,7 @@ func intersectionCountArrayBitmap(a, b *Container) (n int32) { } func intersectionCountBitmapBitmap(a, b *Container) (n int32) { + statsHit("intersectionCount/BitmapBitmap") return int32(popcountAndSlice(a.bitmap, b.bitmap)) } @@ -1983,6 +2023,7 @@ func intersect(a, b *Container) *Container { } func intersectArrayArray(a, b *Container) *Container { + statsHit("intersect/ArrayArray") output := &Container{containerType: containerArray} na, nb := len(a.array), len(b.array) for i, j := 0, 0; i < na && j < nb; { @@ -2004,6 +2045,7 @@ func intersectArrayArray(a, b *Container) *Container { // container. The return is always an array container (since it's guaranteed to // be low-cardinality) func intersectArrayRun(a, b *Container) *Container { + statsHit("intersect/ArrayRun") output := &Container{containerType: containerArray} na, nb := len(a.array), len(b.runs) for i, j := 0, 0; i < na && j < nb; { @@ -2023,6 +2065,7 @@ func intersectArrayRun(a, b *Container) *Container { // intersectRunRun computes the intersect of two run containers. func intersectRunRun(a, b *Container) *Container { + statsHit("intersect/RunRun") output := &Container{containerType: containerRun} na, nb := len(a.runs), len(b.runs) for i, j := 0, 0; i < na && j < nb; { @@ -2062,6 +2105,7 @@ func intersectRunRun(a, b *Container) *Container { // intersectBitmapRun returns an array container if the run container's // cardinality is < ArrayMaxSize. Otherwise it returns a bitmap container. func intersectBitmapRun(a, b *Container) *Container { + statsHit("intersect/BitmapRun") var output *Container if b.n < ArrayMaxSize { // output is array container @@ -2125,6 +2169,7 @@ func intersectBitmapRun(a, b *Container) *Container { } func intersectArrayBitmap(a, b *Container) *Container { + statsHit("intersect/ArrayBitmap") output := &Container{containerType: containerArray} for _, va := range a.array { bmidx := va / 64 @@ -2140,6 +2185,7 @@ func intersectArrayBitmap(a, b *Container) *Container { } func intersectBitmapBitmap(a, b *Container) *Container { + statsHit("intersect/BitmapBitmap") // local variables added to prevent BCE checks in loop // see https://go101.org/article/bounds-check-elimination.html var ( @@ -2191,6 +2237,7 @@ func union(a, b *Container) *Container { } func unionArrayArray(a, b *Container) *Container { + statsHit("union/ArrayArray") output := &Container{containerType: containerArray} na, nb := len(a.array), len(b.array) for i, j := 0, 0; ; { @@ -2224,6 +2271,7 @@ func unionArrayArray(a, b *Container) *Container { // unionArrayRun optimistically assumes that the result will be a run container, // and converts to a bitmap or array container afterwards if necessary. func unionArrayRun(a, b *Container) *Container { + statsHit("union/ArrayRun") if b.n == maxContainerVal+1 { return b.Clone() } @@ -2281,6 +2329,7 @@ func (c *Container) runAppendInterval(v interval16) int32 { } func unionRunRun(a, b *Container) *Container { + statsHit("union/RunRun") if a.n == maxContainerVal+1 { return a.Clone() } @@ -2315,6 +2364,7 @@ func unionRunRun(a, b *Container) *Container { } func unionBitmapRun(a, b *Container) *Container { + statsHit("union/BitmapRun") if b.n == maxContainerVal+1 { return b.Clone() } @@ -2503,6 +2553,7 @@ func difference(a, b *Container) *Container { // differenceArrayArray computes the difference bween two arrays. func differenceArrayArray(a, b *Container) *Container { + statsHit("difference/ArrayArray") output := &Container{containerType: containerArray} na, nb := len(a.array), len(b.array) for i, j := 0, 0; i < na; { @@ -2528,6 +2579,7 @@ func differenceArrayArray(a, b *Container) *Container { // differenceArrayRun computes the difference of an array from a run. func differenceArrayRun(a, b *Container) *Container { + statsHit("difference/ArrayRun") // func (ac *arrayContainer) iandNotRun16(rc *runContainer16) container { if a.n == 0 || b.n == 0 { @@ -2585,6 +2637,7 @@ func differenceArrayRun(a, b *Container) *Container { // differenceBitmapRun computes the difference of an bitmap from a run. func differenceBitmapRun(a, b *Container) *Container { + statsHit("difference/BitmapRun") if a.n == 0 || b.n == 0 { return a.Clone() } @@ -2599,6 +2652,7 @@ func differenceBitmapRun(a, b *Container) *Container { // differenceRunArray subtracts the bits in an array container from a run // container. func differenceRunArray(a, b *Container) *Container { + statsHit("difference/RunArray") if a.n == 0 || b.n == 0 { return a.Clone() } @@ -2654,6 +2708,7 @@ RUNLOOP: // differenceRunBitmap computes the difference of an run from a bitmap. func differenceRunBitmap(a, b *Container) *Container { + statsHit("difference/RunBitmap") // If a is full, difference is the flip of b. if len(a.runs) > 0 && a.runs[0].start == 0 && a.runs[0].last == 65535 { return flipBitmap(b) @@ -2711,6 +2766,7 @@ func differenceRunBitmap(a, b *Container) *Container { // differenceRunRun computes the difference of two runs. func differenceRunRun(a, b *Container) *Container { + statsHit("difference/RunRun") if a.n == 0 || b.n == 0 { return a.Clone() } @@ -2774,6 +2830,7 @@ func differenceRunRun(a, b *Container) *Container { } func differenceArrayBitmap(a, b *Container) *Container { + statsHit("difference/ArrayBitmap") output := &Container{containerType: containerArray} for _, va := range a.array { bmidx := va / 64 @@ -2790,6 +2847,7 @@ func differenceArrayBitmap(a, b *Container) *Container { } func differenceBitmapArray(a, b *Container) *Container { + statsHit("difference/BitmapArray") output := a.Clone() for _, v := range b.array { @@ -2805,6 +2863,7 @@ func differenceBitmapArray(a, b *Container) *Container { } func differenceBitmapBitmap(a, b *Container) *Container { + statsHit("difference/BitmapBitmap") // local variables added to prevent BCE checks in loop // see https://go101.org/article/bounds-check-elimination.html @@ -2862,6 +2921,7 @@ func xor(a, b *Container) *Container { } func xorArrayArray(a, b *Container) *Container { + statsHit("xor/ArrayArray") output := &Container{containerType: containerArray} na, nb := len(a.array), len(b.array) for i, j := 0, 0; i < na || j < nb; { @@ -2891,6 +2951,7 @@ func xorArrayArray(a, b *Container) *Container { } func xorArrayBitmap(a, b *Container) *Container { + statsHit("xor/ArrayBitmap") output := b.Clone() for _, v := range a.array { if b.bitmapContains(v) { @@ -2910,6 +2971,7 @@ func xorArrayBitmap(a, b *Container) *Container { } func xorBitmapBitmap(a, b *Container) *Container { + statsHit("xor/BitmapBitmap") // local variables added to prevent BCE checks in loop // see https://go101.org/article/bounds-check-elimination.html @@ -2987,6 +3049,7 @@ func (op *op) UnmarshalBinary(data []byte) error { if len(data) < op.size() { return fmt.Errorf("op data out of bounds: len=%d", len(data)) } + statsHit("op/UnmarshalBinary") // Verify checksum. h := fnv.New32a() @@ -3011,6 +3074,7 @@ func lowbits(v uint64) uint16 { return uint16(v & 0xFFFF) } // search32 returns the index of value in a. If value is not found, it works the // same way as search64. func search32(a []uint16, value uint16) int32 { + statsHit("search32") // Optimize for elements and the last element. n := int32(len(a)) if n == 0 { @@ -3054,6 +3118,7 @@ func search32(a []uint16, value uint16) int32 { // since negative 0 is no different from positive 0, we offset the returned // negative indices by 1. See the test for this function for examples. func search64(a []uint64, value uint64) int { + statsHit("search64") // Optimize for elements and the last element. n := len(a) if n == 0 { @@ -3132,6 +3197,7 @@ func (a *ErrorList) AppendWithPrefix(err error, prefix string) { // xorArrayRun computes the exclusive or of an array and a run container. func xorArrayRun(a, b *Container) *Container { + statsHit("xor/ArrayRun") output := &Container{containerType: containerRun} na, nb := len(a.array), len(b.runs) var vb interval16 @@ -3290,6 +3356,7 @@ type xorstm struct { // xorRunRun computes the exclusive or of two run containers. func xorRunRun(a, b *Container) *Container { + statsHit("xor/RunRun") na, nb := len(a.runs), len(b.runs) if na == 0 { return b.Clone() @@ -3338,6 +3405,7 @@ func xorRunRun(a, b *Container) *Container { // xorRunRun computes the exclusive or of a bitmap and a run container. func xorBitmapRun(a, b *Container) *Container { + statsHit("xor/BitmapRun") output := a.Clone() for j := 0; j < len(b.runs); j++ { output.bitmapXorRange(uint64(b.runs[j].start), uint64(b.runs[j].last)+1) @@ -3352,6 +3420,7 @@ func xorBitmapRun(a, b *Container) *Container { } func bitmapsEqual(b, c *Bitmap) error { // nolint: deadcode + statsHit("bitmapsEqual") if b.OpWriter != c.OpWriter { return errors.New("opWriters not equal") } @@ -3404,6 +3473,7 @@ const ( ) func readOfficialHeader(buf []byte) (size uint32, containerTyper func(index uint, card int) byte, header, pos int, haveRuns bool, err error) { + statsHit("readOfficialHeader") if len(buf) < 8 { err = fmt.Errorf("buffer too small, expecting at least 8 bytes, was %d", len(buf)) return size, containerTyper, header, pos, haveRuns, err @@ -3469,6 +3539,7 @@ func (b *Bitmap) UnmarshalBinary(data []byte) error { // Nothing to unmarshal return nil } + statsHit("Bitmap/UnmarshalBinary") fileMagic := uint32(binary.LittleEndian.Uint16(data[0:2])) if fileMagic == magicNumber { // if pilosa roaring return errors.Wrap(b.unmarshalPilosaRoaring(data), "unmarshaling as pilosa roaring") diff --git a/roaring/roaring_nop_stats.go b/roaring/roaring_nop_stats.go new file mode 100644 index 000000000..c9e029ab7 --- /dev/null +++ b/roaring/roaring_nop_stats.go @@ -0,0 +1,8 @@ +// +build !roaringstats + +package roaring + +// statsCount does nothing, because you aren't building with +// the "roaringstats" build tag. +func statsHit(string) { +} diff --git a/roaring/roaring_stats.go b/roaring/roaring_stats.go new file mode 100644 index 000000000..fd8ade91f --- /dev/null +++ b/roaring/roaring_stats.go @@ -0,0 +1,15 @@ +// +build roaringstats + +package roaring + +import ( + "github.com/pilosa/pilosa/stats" +) + +var statsEv = stats.NewExpvarStatsClient() + +// statsHit increments the given stat, so we can tell how often we've hit +// that particular event. +func statsHit(name string) { + statsEv.Count(name, 1, 1) +} From 8e270f9201822605ab1424254920547121dd8eb0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 15 Nov 2018 14:58:07 -0600 Subject: [PATCH 3/3] provide commented-out test case for bug in dead code bitmapEquals isn't currently being called ever, but it has an arcane edge-case bug, so I've made the test case for it and commented it out for future reference. --- roaring/roaring_internal_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index a4c162629..266bf39e7 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -3261,3 +3261,26 @@ func TestUnmarshalOfficialRoaring(t *testing.T) { } } + +/* +// This function exercises an arcane edge case in dead code. +// It doesn't need to be run right now. +func TestEquals(t *testing.T) { + bma := NewBitmap() + bmr := NewBitmap() + for i := uint64(0); i < 30; i++ { + bma.Add(i) + bmr.Add(i) + } + bmr.Optimize() + bmi := bma.Intersect(bmr) + err := bitmapsEqual(bmi, bma) + if err != nil { + t.Fatalf("expected intersection to equal array") + } + err = bitmapsEqual(bmi, bmr) + if err != nil { + t.Fatalf("expected intersection to equal run") + } +} +*/