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

Use efficientgo/core/errors consistently #134

Merged
merged 1 commit into from Dec 2, 2022
Merged

Use efficientgo/core/errors consistently #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 2 deletions Makefile
View file
Open in desktop
Expand Up @@ -62,11 +62,12 @@ format: $(GOIMPORTS)
lint: format deps $(GOLANGCI_LINT) $(FAILLINT) $(COPYRIGHT) docs
$(call require_clean_work_tree,'detected not clean work tree before running lint, previous job changed something?')
@echo ">> verifying modules being imported"
@# TODO(bwplotka): Add, Printf, DefaultRegisterer, NewGaugeFunc and MustRegister once exception are accepted. Add fmt.{Errorf}=github.com/pkg/errors.{Errorf} once https://github.com/fatih/faillint/issues/10 is addressed.
@# TODO(bwplotka): Add, Printf, DefaultRegisterer, NewGaugeFunc and MustRegister once exception are accepted.
@$(FAILLINT) -paths "errors=github.com/efficientgo/core/errors,\
fmt.{Errorf}=github.com/efficientgo/core/errors.{Wrap,Wrapf},\
github.com/prometheus/prometheus/pkg/testutils=github.com/efficientgo/core/testutil,\
github.com/stretchr/testify=github.com/efficientgo/core/testutil" ./...
@$(FAILLINT) -paths "fmt.{Print,Println,Sprint}" -ignore-tests ./...
@$(FAILLINT) -paths "fmt.{Print,Println,Sprint,Errorf}" -ignore-tests ./...
@echo ">> linting all of the Go files GOGC=${GOGC}"
@$(GOLANGCI_LINT) run
@echo ">> ensuring Copyright headers"
Expand Down
3 changes: 1 addition & 2 deletions engine/engine.go
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@ package engine

import (
"context"
"fmt"
"io"
"math"
"runtime"
Expand Down Expand Up @@ -363,7 +362,7 @@ func recoverEngine(logger log.Logger, expr parser.Expr, errp *error) {
buf = buf[:runtime.Stack(buf, false)]

level.Error(logger).Log("msg", "runtime panic in engine", "expr", expr.String(), "err", e, "stacktrace", string(buf))
*errp = fmt.Errorf("unexpected error: %w", err)
*errp = errors.Wrap(err, "unexpected error")
}
}

Expand Down
3 changes: 2 additions & 1 deletion execution/aggregate/khashaggregate.go
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import (
"sort"
"sync"

"github.com/efficientgo/core/errors"
"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/promql/parser"
"golang.org/x/exp/slices"
Expand Down Expand Up @@ -100,7 +101,7 @@ func (a *kAggregate) Next(ctx context.Context) ([]model.StepVector, error) {
a.paramOp.GetPool().PutVectors(args)

if len(args) != len(in) {
return nil, fmt.Errorf("scalar argument not found")
return nil, errors.New("scalar argument not found")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the advantage of using errors.New instead of fmt.Errorf?

Copy link
Member Author

Choose a reason for hiding this comment

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

This lib has stacktraces + some other advantages in API over Go stdlib one. It was implemented in Thanos first and later ported to efficientgo/core. For more details: thanos-io/thanos#5176 and thanos-io/thanos#5239

Copy link
Member Author

Choose a reason for hiding this comment

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

Also effort on Thanos to port entire codebase to it: thanos-io/thanos#5382 (although now we might do it from efficientgo/core

}

a.once.Do(func() { err = a.init(ctx) })
Expand Down
3 changes: 2 additions & 1 deletion execution/binary/vector.go
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"sync"

"github.com/efficientgo/core/errors"
"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/promql/parser"
"golang.org/x/exp/slices"
Expand Down Expand Up @@ -193,7 +194,7 @@ func (o *vectorOperator) Next(ctx context.Context) ([]model.StepVector, error) {
group := sampleID.MatchLabels(o.matching.On, o.matching.MatchingLabels...)
msg := "found duplicate series for the match group %s on the %s hand-side of the operation: [%s, %s]" +
";many-to-many matching not allowed: matching labels must be unique on one side"
return nil, fmt.Errorf(msg, group, err.side, sampleID.String(), duplicateSampleID.String())
return nil, errors.Newf(msg, group, err.side, sampleID.String(), duplicateSampleID.String())
}
o.lhs.GetPool().PutStepVector(vector)
}
Expand Down
5 changes: 3 additions & 2 deletions execution/scan/vector_selector.go
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ import (
"sync"
"time"

"github.com/efficientgo/core/errors"
"github.com/prometheus/prometheus/tsdb/chunkenc"

"github.com/thanos-community/promql-engine/execution/model"
Expand All @@ -21,7 +22,7 @@ import (
"github.com/prometheus/prometheus/storage"
)

var ErrNativeHistogramsUnsupported = fmt.Errorf("querying native histograms is not supported")
var ErrNativeHistogramsUnsupported = errors.Newf("querying native histograms is not supported")

type vectorScanner struct {
labels labels.Labels
Expand Down Expand Up @@ -178,7 +179,7 @@ func selectPoint(it *storage.MemoizedSeriesIterator, ts, lookbackDelta, offset i
case chunkenc.ValFloat:
t, v = it.At()
default:
panic(fmt.Errorf("unknown value type %v", valueType))
panic(errors.Newf("unknown value type %v", valueType))
}
if valueType == chunkenc.ValNone || t > refTime {
var ok bool
Expand Down