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.

Sign up for GitHub

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

Jump to bottom

Proper enrichFromCache panic fix and go-whisper upgrade #486

Merged
merged 8 commits into from
Aug 11, 2022
Merged

Proper enrichFromCache panic fix and go-whisper upgrade #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8c89201
fix(go-carbon): proper fix for enrichFromCache panic
deniszh Aug 11, 2022
3debc0d
fix(go-whisper): upgrading go-whisper for buffer fix
deniszh Aug 11, 2022
363ba22
Upgrade golangci-lint
deniszh Aug 11, 2022
0bf9d12
Another upgrade of go-whisper
deniszh Aug 11, 2022
25be9c2
Adding protective code back
deniszh Aug 11, 2022
eb5c8d0
Fixing gofmt linter
deniszh Aug 11, 2022
f0ed6c9
taming staticcheck linter
deniszh Aug 11, 2022
29e8ee1
taming staticcheck linter
deniszh Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions .github/workflows/golangci-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,13 @@ jobs:
uses: golangci/golangci-lint-action@v2
with:
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
version: v1.45.2
version: v1.48.0

# Optional: working directory, useful for monorepos
# working-directory: somedir

# Optional: golangci-lint command line arguments.
# TODO: --go 1.17 is added because of https://github.com/golangci/golangci-lint/issues/2649#issuecomment-1070528726
args: --enable gofmt,goimports,gocritic --disable errcheck --go 1.17
args: --enable gofmt,goimports,gocritic --disable errcheck

# Optional: show only new issues if it's a pull request. The default value is `false`.
# only-new-issues: true
2 changes: 1 addition & 1 deletion carbon/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package carbon
import (
"errors"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"log"
"net"
"net/url"
Expand Down
2 changes: 1 addition & 1 deletion carbon/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package carbon
import (
"bytes"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"log"
"path/filepath"
"strings"
Expand Down
2 changes: 1 addition & 1 deletion carbon/grace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package carbon
import (
"bufio"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"os"
"path"
"sort"
Expand Down
2 changes: 1 addition & 1 deletion carbon/restore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package carbon

import (
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"path"
"testing"

Expand Down
2 changes: 1 addition & 1 deletion carbonserver/cache_index_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package carbonserver
import (
"context"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"os"
"path/filepath"
"testing"
Expand Down
2 changes: 1 addition & 1 deletion carbonserver/capability.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package carbonserver

import (
"encoding/json"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"net/http"
_ "net/http/pprof"
"os"
Expand Down
2 changes: 1 addition & 1 deletion carbonserver/carbonserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package carbonserver

import (
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"math"
"os"
"path/filepath"
Expand Down
11 changes: 10 additions & 1 deletion carbonserver/fetchsinglemetric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,23 @@ func (r response) enrichFromCache(listener *CarbonserverListener, cacheData []po
atomic.AddUint64(&listener.metrics.CacheRequestsTotal, 1)
cacheStartTime := time.Now()
pointsFetchedFromCache := 0
// extend values array if needed
max_index := (r.StopTime - r.StartTime) / r.StepTime
start_index := int64(len(r.Values))
if start_index < max_index {
for i := start_index; i < max_index; i++ {
// math.NaN is not working here
// values will be replaced when merging with cache
r.Values = append(r.Values, 0)
}
}
for _, item := range cacheData {
ts := int64(item.Timestamp) - int64(item.Timestamp)%r.StepTime
if ts < r.StartTime || ts >= r.StopTime {
continue
}
pointsFetchedFromCache++
index := (ts - r.StartTime) / r.StepTime
// TODO: log.debug such cases
if index >= 0 && index < int64(len(r.Values)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about keeping the defensive code just in case?
This way, a bug or invalid data in the cache (like ts older than r.StartTime) will not cause a panic.
But I'm not well aware of the code as a whole. So your call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it will not hurt, so let's keep it.

r.Values[index] = item.Value
}
Expand Down
2 changes: 1 addition & 1 deletion carbonserver/find.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"net/http"
_ "net/http/pprof"
"strings"
Expand Down
2 changes: 1 addition & 1 deletion carbonserver/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package carbonserver

import (
"encoding/json"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"net/http"
_ "net/http/pprof"
"strings"
Expand Down
2 changes: 1 addition & 1 deletion carbonserver/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"math"
"net/http"
_ "net/http/pprof"
Expand Down
16 changes: 10 additions & 6 deletions carbonserver/trie.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ func (g *gdstate) matched() bool {
}

// TODO:
// add range value validation
//
// add range value validation
func newGlobState(expr string, expand func(globs []string) ([]string, error)) (*gmatcher, error) {
var m = gmatcher{root: &gstate{}, exact: true, expr: expr}
var cur = m.root
Expand Down Expand Up @@ -1661,14 +1662,17 @@ func (q *throughputUsagePerNamespace) increase(x int64) {
// throughputUsagePerNamespace.offset throughputUsagePerNamespace.withinQuota
// dataPoints -> 1024
// resetAt -> now-1m
// dataPoints -> 1024
//
// dataPoints -> 1024
//
// dataPoints -> 0
// resetAt -> now
// resetAt -> now
//
// incorrect quota over enforcement as
// dataPoints is already 0 by the time
// withinQuota retrieves resetAt
// resetAt -> now
//
// incorrect quota over enforcement as
// dataPoints is already 0 by the time
// withinQuota retrieves resetAt
//
// the downside of this approach is that go-carbon might still be updating on
// the counter in the replaced recorder, causing some mis-report or
Expand Down
3 changes: 2 additions & 1 deletion carbonserver/trie_real_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//go:build real
// +build real

package carbonserver
Expand All @@ -6,7 +7,7 @@ import (
"context"
"flag"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"math/rand"
"reflect"
"runtime/debug"
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/dgryski/go-trigram v0.0.0-20160407183937-79ec494e1ad0
github.com/dgryski/httputil v0.0.0-20160116060654-189c2918cd08
github.com/go-graphite/carbonzipper v0.0.0-20180329125635-fedce067a794
github.com/go-graphite/go-whisper v0.0.0-20220706140940-0fdbe10fe673
github.com/go-graphite/go-whisper v0.0.0-20220811123944-7ae7056cf3c0
github.com/go-graphite/protocol v1.0.1-0.20220718132526-4b842ba389ee
github.com/gogo/protobuf v1.3.2
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db // indirect
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
github.com/go-graphite/carbonzipper v0.0.0-20180329125635-fedce067a794 h1:9N+1I8z47huAZdcBWVIqfZZPzIzMyXqGd3uR21QN5mA=
github.com/go-graphite/carbonzipper v0.0.0-20180329125635-fedce067a794/go.mod h1:sfZ+AkP8/bBcWSGVqhseV6t6e/+C0i/PkTpA32W5rXs=
github.com/go-graphite/go-whisper v0.0.0-20220706140940-0fdbe10fe673 h1:KXBNR5tGEMaq2ZOtUNgRdPEJc5LfNFUQj8LGsaRyeXA=
github.com/go-graphite/go-whisper v0.0.0-20220706140940-0fdbe10fe673/go.mod h1:1edRhfqJoiHSjN72nYptHK0YR4yYt8aPC4NRHWhT0XE=
github.com/go-graphite/go-whisper v0.0.0-20220811123944-7ae7056cf3c0 h1:eUcmhaQSskwmKVC1kDGypiJR2NhalzrIrnzagHh7Cxk=
github.com/go-graphite/go-whisper v0.0.0-20220811123944-7ae7056cf3c0/go.mod h1:1edRhfqJoiHSjN72nYptHK0YR4yYt8aPC4NRHWhT0XE=
github.com/go-graphite/protocol v1.0.1-0.20220718132526-4b842ba389ee h1:Sb63UbLUOXtKFIfNfZIkWrs73vWD0ZKzrBdTjvlVOBQ=
github.com/go-graphite/protocol v1.0.1-0.20220718132526-4b842ba389ee/go.mod h1:puWkW2DFZi46CaLY1rxYhkDiBDFlpaTDaMjcE/Cd9gw=
github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
Expand Down
2 changes: 1 addition & 1 deletion helper/atomicfiles/atomicfiles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package atomicfiles

import (
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"os"
"path"
)
Expand Down
2 changes: 1 addition & 1 deletion helper/qa/tmp_dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package qa

import (
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"os"
"testing"
)
Expand Down
2 changes: 1 addition & 1 deletion persister/ini.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package persister
import (
"bytes"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"strings"
)

Expand Down
12 changes: 6 additions & 6 deletions persister/throttle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import (

// ThrottleTicker is a ticker that can be used for hard or soft rate-limiting.
//
// * A soft rate limiter will send a message on C at the actual rate that is
// specified.
// - A soft rate limiter will send a message on C at the actual rate that is
// specified.
//
// * A hard rate limiter may send arbitrarily many messages on C every second,
// but it will send the value 'true' with the first ratePerSec ones, and
// 'false' with all subsequent ones, until the next second. It is up to the
// user to decide what to do in each case.
// - A hard rate limiter may send arbitrarily many messages on C every second,
// but it will send the value 'true' with the first ratePerSec ones, and
// 'false' with all subsequent ones, until the next second. It is up to the
// user to decide what to do in each case.
type ThrottleTicker struct {
helper.Stoppable
C chan bool
Expand Down
2 changes: 1 addition & 1 deletion persister/whisper_schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package persister

import (
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"os"
"testing"

Expand Down
3 changes: 2 additions & 1 deletion points/points.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ func (p *Points) WriteBinaryTo(w io.Writer) (n int, err error) {
}

// ParseText parse text protocol Point
// host.Point.value 42 1422641531\n
//
// host.Point.value 42 1422641531\n
func ParseText(line string) (*Points, error) {

row := strings.Split(strings.Trim(line, "\n \t\r"), " ")
Expand Down
2 changes: 1 addition & 1 deletion receiver/http/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package http

import (
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"net"
"net/http"
"sync/atomic"
Expand Down
2 changes: 1 addition & 1 deletion receiver/kafka/kafka.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

"encoding/json"
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"strings"
"sync"
"time"
Expand Down
2 changes: 1 addition & 1 deletion receiver/pubsub/pubsub.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"context"
"fmt"
"io"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"sync"
"sync/atomic"
"time"
Expand Down
2 changes: 1 addition & 1 deletion tags/tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package tags

import (
"fmt"
"io/ioutil"
"io/ioutil" //nolint:staticcheck
"net/http"
"net/url"
"sync/atomic"
Expand Down
8 changes: 5 additions & 3 deletions vendor/github.com/go-graphite/go-whisper/whisper.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ github.com/eapache/queue
# github.com/go-graphite/carbonzipper v0.0.0-20180329125635-fedce067a794
## explicit
github.com/go-graphite/carbonzipper/zipper/httpHeaders
# github.com/go-graphite/go-whisper v0.0.0-20220706140940-0fdbe10fe673
# github.com/go-graphite/go-whisper v0.0.0-20220811123944-7ae7056cf3c0
## explicit; go 1.18
github.com/go-graphite/go-whisper
# github.com/go-graphite/protocol v1.0.1-0.20220718132526-4b842ba389ee
Expand Down