Cleans up pkg/storage/segment and global state (#183)

* cleans up segment and global state

Author: petethepig

.github/workflows/lint-go.yml

@@ -0,0 +1,19 @@
+name: Go Linting
+
+on:
+  push:
+    branches: [main]
+  pull_request:
+    branches: [main]
+
+jobs:
+  go-lint:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+      - name: Run revive action
+        uses: petethepig/revive-action@v5
+        with:
+          config: revive.toml
+          exclude: "vendor/..."

Makefile

@@ -5,6 +5,7 @@ ifeq ("$(shell go env GOARCH || true)", "arm64")
 	GODEBUG=asyncpreemptoff=1
 endif
 
+ALL_SPIES = "ebpfspy,rbspy,pyspy,debugspy"
 ifeq ("$(shell go env GOOS || true)", "linux")
 	ENABLED_SPIES ?= "ebpfspy,rbspy,pyspy"
 else
@@ -79,7 +80,7 @@ ensure-logrus-not-used:
 
 .PHONY: unused
 unused:
-	staticcheck -f stylish -unused.whole-program ./...
+	staticcheck -f stylish -tags $(ALL_SPIES) -unused.whole-program ./...
 
 .PHONY: install-dev-tools
 install-dev-tools:

cmd/pyroscope/main.go

@@ -9,7 +9,7 @@ import (
 )
 
 func main() {
-	cfg := config.New()
+	cfg := &config.Config{}
 	err := cli.Start(cfg)
 	if err != nil {
 		os.Stderr.Write([]byte(color.RedString("Error: ") + err.Error() + "\n\n"))

pkg/config/config.go

@@ -50,11 +50,7 @@ type Server struct {
 
 	// TODO: I don't think a lot of people will change these values.
 	//   I think these should just be constants.
-	Multiplier       int           `skip:"true" def:"10"`
-	MinResolution    time.Duration `skip:"true" def:"10s"`
-	MaxResolution    time.Duration `skip:"true" def:"8760h"` // 365 days
-	StorageMaxDepth  int           `skip:"true"`
-	BadgerNoTruncate bool          `def:"false" desc:"indicates whether value log files should be truncated to delete corrupt data, if any"`
+	BadgerNoTruncate bool `def:"false" desc:"indicates whether value log files should be truncated to delete corrupt data, if any"`
 
 	MaxNodesSerialization int `def:"2048" desc:"max number of nodes used when saving profiles to disk"`
 	MaxNodesRender        int `def:"8192" desc:"max number of nodes used to display data on the frontend"`
@@ -96,44 +92,3 @@ type Exec struct {
 	GroupName              string        `def:"" desc:"starts process under specified group name"`
 	PyspyBlocking          bool          `def:"false" desc:"enables blocking mode for pyspy"`
 }
-
-func calculateMaxDepth(min, max time.Duration, multiplier int) int {
-	depth := 0
-	for min < max {
-		min *= time.Duration(multiplier)
-		depth++
-	}
-	return depth
-}
-
-// TODO: remove these preset configs
-func New() *Config {
-	return NewForTests("/tmp/")
-}
-
-func NewForTests(path string) *Config {
-	cfg := &Config{
-		Server: Server{
-			StoragePath: path,
-			APIBindAddr: ":4040",
-
-			CacheSegmentSize:    10,
-			CacheTreeSize:       10,
-			CacheDictionarySize: 10,
-			CacheDimensionSize:  10,
-
-			Multiplier:    10,
-			MinResolution: 10 * time.Second,
-			MaxResolution: time.Hour * 24 * 365 * 5,
-
-			MaxNodesSerialization: 2048,
-			MaxNodesRender:        2048,
-
-			OutOfSpaceThreshold: 512 * 1024 * 1024, // bytes (default: 512MB)
-		},
-	}
-
-	cfg.Server.StorageMaxDepth = calculateMaxDepth(cfg.Server.MinResolution, cfg.Server.MaxResolution, cfg.Server.Multiplier)
-
-	return cfg
-}

pkg/storage/segment/constants.go

@@ -0,0 +1,21 @@
+package segment
+
+import "time"
+
+// TODO: at some point we should change it so that segments can support different
+// resolution and multiplier values. For now they are constants
+const (
+	multiplier = 10
+	resolution = 10 * time.Second
+)
+
+var durations = []time.Duration{}
+
+func init() {
+	d := resolution
+	// TODO: better upper boundary, currently 50 is a magic number
+	for i := 0; i < 50; i++ {
+		durations = append(durations, d)
+		d *= time.Duration(multiplier)
+	}
+}

pkg/storage/segment/fuzz_test.go

@@ -54,7 +54,7 @@ func (sm *storageMock) Get(st, et time.Time, cb func(depth int, samples, writes
 
 // if you change something in this test make sure it doesn't change test coverage.
 func fuzzTest(testWrites bool, writeSize func() int) {
-	s := New(10*time.Second, 10)
+	s := New()
 	m := newMock(10 * time.Second)
 
 	r := rand.New(rand.NewSource(1213))

pkg/storage/segment/segment.go

@@ -16,8 +16,6 @@ type streeNode struct {
 	children []*streeNode
 }
 
-var durations []time.Duration
-
 func (parent *streeNode) replace(child *streeNode) {
 	i := child.time.Sub(parent.time) / durations[child.depth]
 	parent.children[i] = child
@@ -170,26 +168,12 @@ func newNode(t time.Time, depth, multiplier int) *streeNode {
 	return sn
 }
 
-// TODO: global state is not good
-func InitializeGlobalState(resolution time.Duration, multiplier int) {
-	// this is here just to initialize global duration variable
-	New(resolution, multiplier)
-}
-
-func New(resolution time.Duration, multiplier int) *Segment {
+func New() *Segment {
 	st := &Segment{
 		resolution: resolution,
 		multiplier: multiplier,
-		durations:  []time.Duration{},
-	}
-
-	// TODO better upper boundary
-	d := resolution
-	for i := 0; i < 50; i++ {
-		st.durations = append(st.durations, d)
-		d *= time.Duration(multiplier)
+		durations:  durations,
 	}
-	durations = st.durations
 
 	return st
 }

pkg/storage/segment/segment_test.go

@@ -47,13 +47,10 @@ func expectChildrenSamplesAddUpToParentSamples(tn *streeNode) {
 }
 
 var _ = Describe("stree", func() {
-	r := 10 * time.Second
-	m := 10
-
 	Context("Get", func() {
 		Context("When there's no root", func() {
 			It("get doesn't fail", func() {
-				s := New(r, m)
+				s := New()
 				Expect(doGet(s, testing.SimpleTime(0), testing.SimpleTime(39))).To(HaveLen(0))
 			})
 		})
@@ -64,7 +61,7 @@ var _ = Describe("stree", func() {
 			Context("When second insert is far in the future", func() {
 				It("sets root properly", func() {
 					log.Println("---")
-					s := New(r, m)
+					s := New()
 					s.Put(testing.SimpleTime(1330),
 						testing.SimpleTime(1339), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 					Expect(s.root).ToNot(BeNil())
@@ -77,7 +74,7 @@ var _ = Describe("stree", func() {
 			Context("When second insert is far in the past", func() {
 				It("sets root properly", func() {
 					log.Println("---")
-					s := New(r, m)
+					s := New()
 					s.Put(testing.SimpleTime(2030),
 						testing.SimpleTime(2039), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 					Expect(s.root).ToNot(BeNil())
@@ -91,31 +88,31 @@ var _ = Describe("stree", func() {
 
 		Context("When empty", func() {
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(0),
 					testing.SimpleTime(9), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
 				Expect(s.root.depth).To(Equal(0))
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(0),
 					testing.SimpleTime(49), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
 				Expect(s.root.depth).To(Equal(1))
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(10),
 					testing.SimpleTime(109), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
 				Expect(s.root.depth).To(Equal(2))
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(10),
 					testing.SimpleTime(19), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
@@ -126,7 +123,7 @@ var _ = Describe("stree", func() {
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(10),
 					testing.SimpleTime(19), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
@@ -139,7 +136,7 @@ var _ = Describe("stree", func() {
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(10),
 					testing.SimpleTime(19), 10, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
@@ -152,7 +149,7 @@ var _ = Describe("stree", func() {
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(10),
 					testing.SimpleTime(19), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
@@ -171,7 +168,7 @@ var _ = Describe("stree", func() {
 			})
 
 			It("sets root properly", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(30),
 					testing.SimpleTime(39), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
@@ -191,7 +188,7 @@ var _ = Describe("stree", func() {
 			})
 
 			It("works with 3 mins", func() {
-				s := New(r, m)
+				s := New()
 				s.Put(testing.SimpleTime(10),
 					testing.SimpleTime(70), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 				Expect(s.root).ToNot(BeNil())
@@ -200,7 +197,7 @@ var _ = Describe("stree", func() {
 			})
 
 			It("sets trie properly, gets work", func() {
-				s := New(r, m)
+				s := New()
 
 				s.Put(testing.SimpleTime(0),
 					testing.SimpleTime(9), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})

pkg/storage/segment/serialization.go

@@ -78,8 +78,8 @@ func (s *Segment) Serialize(w io.Writer) error {
 	return nil
 }
 
-func Deserialize(resolution time.Duration, multiplier int, r io.Reader) (*Segment, error) {
-	s := New(resolution, multiplier)
+func Deserialize(r io.Reader) (*Segment, error) {
+	s := New()
 	br := bufio.NewReader(r) // TODO if it's already a bytereader skip
 
 	// reads serialization format version, see comment at the top
@@ -155,8 +155,8 @@ func (t *Segment) Bytes() []byte {
 	return b.Bytes()
 }
 
-func FromBytes(resolution time.Duration, multiplier int, p []byte) *Segment {
+func FromBytes(p []byte) *Segment {
 	// TODO: handle error
-	t, _ := Deserialize(resolution, multiplier, bytes.NewReader(p))
+	t, _ := Deserialize(bytes.NewReader(p))
 	return t
 }

pkg/storage/segment/serialization_test.go

@@ -21,12 +21,9 @@ var serializedExampleV2 = "\x02={\"aggregationType\":\"\",\"sampleRate\":0,\"spy
 	"\x00\x8a\x92\xb8Ø\xfe\xff\xff\xff\x01\x01\x01\x01\x00\x00\x94\x92\xb8Ø\xfe\xff\xff\xff\x01\x01\x01\x01\x00"
 
 var _ = Describe("stree", func() {
-	r := 10 * time.Second
-	m := 10
-
 	Context("Serialize / Deserialize", func() {
 		It("both functions work properly", func() {
-			s := New(r, m)
+			s := New()
 			s.Put(testing.SimpleTime(0),
 				testing.SimpleTime(9), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 			s.Put(testing.SimpleTime(10),
@@ -39,7 +36,7 @@ var _ = Describe("stree", func() {
 			serialized := buf.Bytes()
 			log.Printf("%q", serialized)
 
-			s, err := Deserialize(r, m, bytes.NewReader(serialized))
+			s, err := Deserialize(bytes.NewReader(serialized))
 			Expect(err).ToNot(HaveOccurred())
 			var buf2 bytes.Buffer
 			s.Serialize(&buf2)
@@ -50,7 +47,7 @@ var _ = Describe("stree", func() {
 
 	Context("Serialize", func() {
 		It("serializes segment tree properly", func() {
-			s := New(r, m)
+			s := New()
 			s.Put(testing.SimpleTime(0),
 				testing.SimpleTime(9), 1, func(de int, t time.Time, r *big.Rat, a []Addon) {})
 			s.Put(testing.SimpleTime(10),
@@ -69,7 +66,7 @@ var _ = Describe("stree", func() {
 	Context("Deserialize", func() {
 		Context("v1", func() {
 			It("deserializes v1 data", func() {
-				s, err := Deserialize(r, m, bytes.NewReader([]byte(serializedExampleV1)))
+				s, err := Deserialize(bytes.NewReader([]byte(serializedExampleV1)))
 				Expect(err).ToNot(HaveOccurred())
 				Expect(s.root.children[0]).ToNot(BeNil())
 				Expect(s.root.children[1]).ToNot(BeNil())
@@ -79,7 +76,7 @@ var _ = Describe("stree", func() {
 		})
 		Context("v2", func() {
 			It("deserializes v2 data", func() {
-				s, err := Deserialize(r, m, bytes.NewReader([]byte(serializedExampleV2)))
+				s, err := Deserialize(bytes.NewReader([]byte(serializedExampleV2)))
 				Expect(err).ToNot(HaveOccurred())
 				Expect(s.root.children[0]).ToNot(BeNil())
 				Expect(s.root.children[1]).ToNot(BeNil())