Refactor collector mains #2060

jpkrohling · 2020-02-07T15:56:51Z

Which problem is this PR solving?

Follow up from Improved graceful shutdown - Agent #2031, refactoring the collector-specific entry points for all-in-one and collector

Short description of the changes

Each commit is a refactoring unit and can be analyzed alone: each commit passes the tests from make test and both all-in-one and collector start successfully
The first 4 commits are in preparation for the fourth, to avoid cyclic imports
The fifth commit is similar to Improved graceful shutdown - Agent #2031, but without some of the graceful shutdown parts
Then, a few changes based on the reviews for Improved graceful shutdown - Agent #2031
The remaining commits are further smaller refactoring, moving the servers to a specific package, plus consolidating the old grpcserver into server/grpc.go.
And finally, split the server's networking parts from the serving parts, so that they are more testable. Tests not included.

I think this PR is already big enough on its own, so, I won't continue the refactoring of all-in-one as part of this commit.

pavolloffay · 2020-02-10T07:56:43Z

cmd/collector/app/collector.go

+}
+
+// Close the component and all its underlying dependencies
+func (c *Collector) Close() error {


You either forget to return error or you can change the signature without error.

The return type is part of the io.Closer interface.

Then return the error. There are several return assignments in this function and none error being returned.

There's no error that we would want to bubble up from here. For instance, when issuing a Close(), we don't want to break the routine when the HTTP server failed to close, as that would result in the Zipkin server not being closed.

It does not mean that the error has to be returned immediatelly. It can be wrapped and returned at the end of the function. Hopefully, this function is not used by many clients...

Interesting idea. If the HTTP server and Zipkin fails, Zipkin will end up wrapping the HTTP server failure. Not sure that's intuitive from the perspective of the admin looking at the logs.

Hopefully, this function is not used by many clients...

It's being used by the main.go of both the Collector and All In One.

cmd/collector/app/processor/span.go

cmd/collector/app/server/grpc.go

cmd/collector/app/server/thrift.go

cmd/collector/app/server/zipkin.go

codecov · 2020-02-10T10:04:45Z

Codecov Report

Merging #2060 into master will decrease coverage by 1.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2060      +/-   ##
==========================================
- Coverage   97.41%   96.38%   -1.04%     
==========================================
  Files         209      214       +5     
  Lines       10351    10525     +174     
==========================================
+ Hits        10083    10144      +61     
- Misses        224      325     +101     
- Partials       44       56      +12

Impacted Files	Coverage Δ
cmd/collector/app/handler/thrift_span_handler.go	`94.73% <0.00%> (ø)`
cmd/collector/app/server/thrift.go	`0.00% <0.00%> (ø)`
cmd/collector/app/server/grpc.go	`67.85% <0.00%> (ø)`
cmd/collector/app/server/http.go	`0.00% <0.00%> (ø)`
cmd/collector/app/server/zipkin.go	`0.00% <0.00%> (ø)`
cmd/collector/app/collector.go	`73.80% <0.00%> (ø)`
cmd/collector/app/server/test.go	`50.00% <0.00%> (ø)`
cmd/query/app/server.go	`94.52% <0.00%> (+2.73%)`	⬆️

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 e2411e5...2e8d8cb. Read the comment docs.

jpkrohling · 2020-02-10T10:10:01Z

Review addressed + conflicts solved.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling · 2020-02-11T10:46:08Z

The rename of processor.SpanProcessor has been reverted and the PR has been rebased.

jpkrohling · 2020-02-11T11:17:39Z

Coverage is failing, which is kinda expected, as the refactored code didn't have any tests at all, except for the gRPC server, which was kept.

pavolloffay

LGTM, except the function signatures which return an error but in reality they do not return any error - serveZipkin, serverThrift

cmd/collector/app/server/http.go

cmd/collector/app/server/zipkin.go

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

cmd/collector/app/processor/span.go

yurishkuro · 2020-02-11T15:42:41Z

cmd/collector/app/server/grpc.go

@@ -0,0 +1,93 @@
+// Copyright (c) 2020 The Jaeger Authors.


This file seems to be replacing cmd/collector/app/grpcserver/grpc_server.go, which was deleted (git didn't recognize it as a move).

cmd/collector/app/server/grpc.go

yurishkuro · 2020-02-11T15:46:25Z

cmd/collector/app/server/grpc.go

+	}
+
+	grpcPortStr := ":" + strconv.Itoa(params.Port)
+	listener, err := net.Listen("tcp", grpcPortStr)


Future TODO: similar to query-service that runs both http and grpc servers on the same port, we could do the same for collector.

cmd/collector/app/server/grpc.go

yurishkuro · 2020-02-11T15:52:39Z

cmd/collector/app/server/http.go

+type HTTPServerParams struct {
+	Port            int
+	Handler         handler.JaegerBatchesHandler
+	RecoveryHandler func(http.Handler) http.Handler


why does this need to be exposed?

Not sure I get the question. The HTTPServerParams instance is created by the Collector#Start in cmd/collector/app...

the recovery handler is a mechanical thing that can be added silently, it's weird to have it exposed via API like this.

cmd/collector/app/server/http.go

cmd/collector/main.go

jpkrohling · 2020-02-12T11:08:25Z

I'll work on @yurishkuro's comments as soon as #2031 is merged, as there's potential for conflicts here.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling requested a review from a team as a code owner February 7, 2020 15:56

jpkrohling requested a review from objectiser February 7, 2020 15:56

pavolloffay reviewed Feb 10, 2020

View reviewed changes

pavolloffay changed the title ~~Refactor mains~~ Refactor collector mains Feb 10, 2020

jpkrohling added 16 commits February 11, 2020 11:36

Moved cmd/collector/app/builder to app/

56fe8d6

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Moved normalizer to its own package

788463a

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Create handler and processor sub packages for collector

c3d8b50

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Moved remaining collector handlers to app/handler/

3242725

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Collector as 'app'

f9bb9bb

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Added CollectorParams

1568192

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Functions for thrift and grpc servers

fa5b00c

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Zipkin server

2b0607f

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

HTTP Server

5b36e04

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Moved servers to a dedicated package

0eb5f41

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Consolidated grpcserver into app/server/grpc

37973cd

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Fixed http port

3dca929

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Split networking from serving in the servers

467caf8

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Lint fixes

ed6e8d4

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Removed superfluous error declaration

3bdda8d

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Reverted type name change from process.Span to processor.SpanProcessor

3fc1af3

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

pavolloffay reviewed Feb 11, 2020

View reviewed changes

cmd/collector/app/server/http.go Outdated Show resolved Hide resolved

cmd/collector/app/server/zipkin.go Show resolved Hide resolved

cmd/collector/app/server/zipkin.go Outdated Show resolved Hide resolved

Removed unused error return types

2e8d8cb

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

pavolloffay approved these changes Feb 11, 2020

View reviewed changes

jpkrohling merged commit 3ae21ef into jaegertracing:master Feb 11, 2020