Replace pkg/errors wrapping with fmt.Errorf #2070

u5surf · 2020-02-13T09:22:40Z

Which problem is this PR solving?

Replace the use of pkg/errors with native error wrapping #2061

Short description of the changes

I replaced error.wrapping using fmt.Errorf

yurishkuro · 2020-02-13T15:16:40Z

cmd/agent/app/builder_test.go

@@ -182,7 +181,7 @@ func (fakeCollectorProxy) EmitBatch(batch *jaeger.Batch) (err error) {
 }

 func (f fakeCollectorProxy) GetSamplingStrategy(serviceName string) (*sampling.SamplingStrategyResponse, error) {
-	return nil, errors.New("no peers available")
+	return nil, fmt.Errorf("no peers available")


there's no need to change this

yurishkuro

There are many unnecessary changes here. The intention was to remove the use of "github.com/pkg/errors", but there's nothing wrong with using the standard errors.New().

To ensure that "github.com/pkg/errors" is gone, we should remove it from Gopkg.toml

codecov · 2020-02-13T16:40:45Z

Codecov Report

Merging #2070 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2070   +/-   ##
=======================================
  Coverage   96.38%   96.38%           
=======================================
  Files         214      214           
  Lines       10525    10529    +4     
=======================================
+ Hits        10144    10148    +4     
  Misses        325      325           
  Partials       56       56

Impacted Files	Coverage Δ
cmd/collector/app/metrics.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/options.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/span_processor.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/zipkin/http_handler.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/builder_flags.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/handler/grpc_handler.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/handler/tchannel_handler.go	`100.00% <0.00%> (ø)`	⬆️
pkg/normalizer/service_name.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/handler/http_handler.go	`100.00% <0.00%> (ø)`	⬆️
cmd/collector/app/handler/thrift_span_handler.go	`94.73% <0.00%> (ø)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ae21ef...c8b5f80. Read the comment docs.

u5surf · 2020-02-13T22:28:35Z

There are many unnecessary changes here. The intention was to remove the use of "github.com/pkg/errors", but there's nothing wrong with using the standard errors.New().

To ensure that "github.com/pkg/errors" is gone, we should remove it from Gopkg.toml

copied

u5surf · 2020-02-14T14:42:58Z

@yurishkuro
I replaced github.com/pkg/errors with native libraries.
then I confirmed following make lint error. It is easy to resolve this error, but some error messages will be changed the non capitalized starting. May I fix all of them?

$ make lint
time staticcheck ./... \
        | grep -v \
                -e model/model.pb.go \
                -e thrift-gen/ \
        >> .lint.log || true

real    0m14.487s
user    0m18.184s
sys     0m2.832s
Detected staticcheck failures:
cmd/agent/app/reporter/grpc/collector_proxy.go:24:2: package "github.com/jaegertracing/jaeger/cmd/agent/app/reporter" is being imported more than once (ST1019)
        cmd/agent/app/reporter/grpc/collector_proxy.go:25:2: other import of "github.com/jaegertracing/jaeger/cmd/agent/app/reporter"
pkg/es/config/config.go:90:15: error strings should not be capitalized (ST1005)
pkg/kafka/auth/config.go:66:10: error strings should not end with punctuation or a newline (ST1005)
plugin/storage/badger/dependencystore/storage_test.go:83:21: unnecessary use of fmt.Sprintf (S1039)
plugin/storage/badger/spanstore/read_write_test.go:430:19: unnecessary use of fmt.Sprintf (S1039)
plugin/storage/badger/spanstore/read_write_test.go:432:18: unnecessary use of fmt.Sprintf (S1039)
plugin/storage/cassandra/spanstore/reader.go:75:25: error strings should not be capitalized (ST1005)
plugin/storage/cassandra/spanstore/reader.go:78:34: error strings should not be capitalized (ST1005)
plugin/storage/cassandra/spanstore/reader.go:81:33: error strings should not be capitalized (ST1005)
plugin/storage/cassandra/spanstore/reader.go:84:30: error strings should not be capitalized (ST1005)
plugin/storage/cassandra/spanstore/reader.go:87:39: error strings should not be capitalized (ST1005)
plugin/storage/cassandra/spanstore/reader.go:90:29: error strings should not be capitalized (ST1005)
plugin/storage/es/dependencystore/storage.go:103:16: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:67:25: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:70:34: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:73:33: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:76:30: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:79:29: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:82:38: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/reader.go:277:16: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/service_operation.go:97:15: error strings should not be capitalized (ST1005)
plugin/storage/es/spanstore/service_operation.go:128:15: error strings should not be capitalized (ST1005)
make: *** [lint-staticcheck] error 1

yurishkuro · 2020-02-14T15:18:57Z

it's ok to change capitalization. but first please see my comment https://github.com/jaegertracing/jaeger/pull/2070/files#r378924852 - you're making too many unnecessary changes by replacing "errors" with "fmt", which is not in scope of this ticket.

yurishkuro · 2020-02-14T16:48:32Z

Gopkg.toml

-[[constraint]]
-  name = "github.com/pkg/errors"
-  version = "^0.8.0"
-


make sure to run dep ensure --update to reflect this change in the lock file.

yurishkuro

looks great, almost there!

yurishkuro · 2020-02-14T16:57:13Z

cmd/query/app/http_handler.go

+	if err != nil {
+		err = fmt.Errorf("unable to parse %s: %w", endTimeParam, err)
+	}
+	if aH.handleError(w, err, http.StatusBadRequest) {


handleError already checks for the error, so these two ifs are redundant. The simplest fix is

if err != nil { aH.handleError(w, fmt.Errorf("unable to parse %s: %w", endTimeParam, err), http.StatusBadRequest) return }

yurishkuro · 2020-02-14T17:16:29Z

plugin/storage/es/spanstore/service_operation.go

@@ -87,14 +87,14 @@ func (s *ServiceOperationStorage) getServices(context context.Context, indices [

 	searchResult, err := searchService.Do(context)
 	if err != nil {
-		return nil, errors.Wrap(err, "Search service failed")
+		return nil, fmt.Errorf("search service failed: %w", err)


nit: search for services failed

yurishkuro · 2020-02-14T17:17:05Z

plugin/storage/es/spanstore/service_operation.go

@@ -118,14 +118,14 @@ func (s *ServiceOperationStorage) getOperations(context context.Context, indices

 	searchResult, err := searchService.Do(context)
 	if err != nil {
-		return nil, errors.Wrap(err, "Search service failed")
+		return nil, fmt.Errorf("search service failed: %w", err)


nit: search for operations failed

yurishkuro · 2020-02-17T04:10:21Z

The linter is breaking due to upgrade of grpc. I am adjusting the deps separately in #2072.

yurishkuro · 2020-02-17T04:11:29Z

NB: you may want to undo the Gopkg.* changes, because pkg/errors is still a transitive dependency, and not having to touch deps will make this PR less complicated.

Signed-off-by: u5surf <u5.horie@gmail.com>

u5surf · 2020-02-17T10:51:01Z

@yurishkuro Certainly, Gopkg.* reverted.

yurishkuro · 2020-02-17T15:04:38Z

Thank you!

u5surf requested a review from a team as a code owner February 13, 2020 09:22

u5surf requested a review from jpkrohling February 13, 2020 09:22

u5surf force-pushed the issue-2061 branch from 9f052db to 58bc946 Compare February 13, 2020 09:24

yurishkuro reviewed Feb 13, 2020

View reviewed changes

u5surf force-pushed the issue-2061 branch from 58bc946 to 9b6bad2 Compare February 13, 2020 16:22

u5surf force-pushed the issue-2061 branch 2 times, most recently from 86c43b7 to 6211b43 Compare February 14, 2020 16:06

yurishkuro reviewed Feb 14, 2020

View reviewed changes

u5surf force-pushed the issue-2061 branch 2 times, most recently from d2c9e42 to 9d05996 Compare February 16, 2020 23:56

yurishkuro approved these changes Feb 17, 2020

View reviewed changes

Replace github.com/pkg/errors with native libraries

c8b5f80

Signed-off-by: u5surf <u5.horie@gmail.com>

u5surf force-pushed the issue-2061 branch from 9d05996 to c8b5f80 Compare February 17, 2020 10:44

yurishkuro merged commit f3df4ef into jaegertracing:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace pkg/errors wrapping with fmt.Errorf #2070

Replace pkg/errors wrapping with fmt.Errorf #2070

u5surf commented Feb 13, 2020

yurishkuro Feb 13, 2020

yurishkuro left a comment

codecov bot commented Feb 13, 2020 •

edited

Loading

u5surf commented Feb 13, 2020

u5surf commented Feb 14, 2020

yurishkuro commented Feb 14, 2020

yurishkuro Feb 14, 2020

yurishkuro left a comment

yurishkuro Feb 14, 2020

yurishkuro Feb 14, 2020

yurishkuro Feb 14, 2020

yurishkuro commented Feb 17, 2020

yurishkuro commented Feb 17, 2020

u5surf commented Feb 17, 2020

yurishkuro commented Feb 17, 2020

Replace pkg/errors wrapping with fmt.Errorf #2070

Replace pkg/errors wrapping with fmt.Errorf #2070

Conversation

u5surf commented Feb 13, 2020

Which problem is this PR solving?

Short description of the changes

yurishkuro Feb 13, 2020

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2020 • edited Loading

Codecov Report

u5surf commented Feb 13, 2020

u5surf commented Feb 14, 2020

yurishkuro commented Feb 14, 2020

yurishkuro Feb 14, 2020

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Feb 14, 2020

Choose a reason for hiding this comment

yurishkuro Feb 14, 2020

Choose a reason for hiding this comment

yurishkuro Feb 14, 2020

Choose a reason for hiding this comment

yurishkuro commented Feb 17, 2020

yurishkuro commented Feb 17, 2020

u5surf commented Feb 17, 2020

yurishkuro commented Feb 17, 2020

codecov bot commented Feb 13, 2020 •

edited

Loading