[ingester] Add metrics& healthcheck, rename Kafka cli flags #1094

ledor473 · 2018-09-28T17:47:03Z

Which problem is this PR solving?

Resolves #1014
Resolves #1013
Resolves #1093

Short description of the changes

Add default healthcheck port (14270)
Add a configurable metrics HTTP handler (default port is 14271)
Make Kafka flags the same as in the Collector (prefix is kafka instead of ingester)

Comments:

I wasn't sure if I should have refactor the following:

Move the various Kafka flags under /pkg
I left it as is mainly because there's a mix of consumer vs producer flags and because the producer flags are under plugins

Also, should the Parallelism be under Kafka prefix? I felt it was more a ingester flags than a kafka one, but I don't have any strong opinion about it

yurishkuro

lgtm

cmd/ingester/app/flags.go

yurishkuro · 2018-09-28T17:52:21Z

cmd/ingester/app/flags.go

@@ -33,6 +33,8 @@ const (

 	// ConfigPrefix is a prefix fro the ingester flags


yurishkuro · 2018-09-28T17:54:22Z

cmd/ingester/app/flags.go

@@ -54,27 +58,33 @@ const (
 	DefaultParallelism = 1000
 	// DefaultEncoding is the default span encoding
 	DefaultEncoding = EncodingProto
+	// DefaultHTTPPort is the default HTTP port


add: "e.g. for /metrics" (I was at first confused why we need http port)

ledor473 · 2018-10-03T16:46:49Z

I'm not sure what would be the best way to fix that problem in the CI. The flags is indeed redefined, but they usually run in different Go applications

--- FAIL: TestKafkaStorage (0.00s)
panic: flag redefined: kafka.brokers [recovered]
	panic: flag redefined: kafka.brokers

I've also fixed the Ingester flag tests and moved ingester.encoding to kafka.encoding because it's like that in Collector

yurishkuro · 2018-10-22T19:03:27Z

The flags is indeed redefined, but they usually run in different Go applications

This sounds like an issue with the tests. We should be able to create a new instance of the FlagSet in the unit test instead of depending on the global one.

ledor473 · 2018-10-22T19:07:15Z

plugin/storage/integration/kafka_test.go

@@ -48,7 +48,7 @@ func (s *KafkaIntegrationTestSuite) initialize() error {
 	topic := "jaeger-kafka-integration-test-" + strconv.FormatInt(time.Now().UnixNano(), 10)

 	f := kafka.NewFactory()
-	v, command := config.Viperize(f.AddFlags, app.AddFlags)
+	v, command := config.Viperize(app.AddFlags)


@yurishkuro the change here fixed the test. app.AddFlags already contained all flags needed by f.InitFromViper(v)

is kafka factory's AddFlags() no-op then? looks a bit odd

I think it's because we are reusing the github.com/jaegertracing/jaeger/plugin/storage/kafka in a context it wasn't meant to be used. Ingester itself doesn't use the Kafka as a storage plugin, but rather as an input

codecov · 2018-10-22T19:44:20Z

Codecov Report

Merging #1094 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1094   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         144     144           
  Lines        6774    6779    +5     
======================================
+ Hits         6774    6779    +5

Impacted Files	Coverage Δ
cmd/ingester/app/flags.go	`100% <100%> (ø)`	⬆️

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 2be7b0f...f466b0a. Read the comment docs.

jpkrohling · 2018-10-30T11:00:09Z

@ledor473 do you still have anything to work here, or can I merge it ?

ledor473 · 2018-10-30T14:02:31Z

@jpkrohling It's ready to merge! I'll rebase it real quick

jpkrohling · 2018-10-30T14:56:25Z

Once codecov turns green, this will be merged.

yurishkuro · 2018-10-30T15:41:45Z

I restarted test coverage job

objectiser · 2018-11-01T09:21:48Z

cmd/ingester/app/flags.go

+	// DefaultHTTPPort is the default HTTP port (e.g. for /metrics)
+	DefaultHTTPPort = 14271
+	// IngesterDefaultHealthCheckHTTPPort is the default HTTP Port for health check
+	IngesterDefaultHealthCheckHTTPPort = 14270


nit: Does this need to have the Ingester prefix - as the other constants don't?

I've taken the variable names from here and both are prefixed:

jaeger/cmd/query/app/flags.go

Line 29 in bbf8b7a

QueryDefaultHealthCheckHTTPPort = 16687

jaeger/cmd/collector/app/builder/builder_flags.go

Line 33 in e93c375

CollectorDefaultHealthCheckHTTPPort = 14269

That being said, there's a prefix on the other variable as well

objectiser · 2018-11-01T09:24:11Z

cmd/ingester/app/flags.go

 	ConfigPrefix = "ingester"
+	// KafkaConfigPrefix is a prefix for the ingester flags


"for the Kafka flags"

cmd/ingester/app/flags.go

pavolloffay · 2018-11-01T17:07:31Z

@ledor473 could you please fix DCO of the last commit? I will merge afterwards.

yurishkuro · 2018-11-01T17:42:49Z

@ledor473 could you please fix DCO of the last commit? I will merge afterwards.

it's a merge issue, unrelated commits got pulled in.

…tp handler and make Kafka flags the same as in the Collector Signed-off-by: Louis-Etienne Dorval <[email protected]>

Signed-off-by: Louis-Etienne Dorval <[email protected]>

ledor473 · 2018-11-01T19:24:53Z

My fault! Should be fixed now

pavolloffay · 2018-11-02T08:44:26Z

thanks @ledor473

ledor473 · 2018-11-15T22:59:51Z

@yurishkuro @pavolloffay @objectiser I've just realized that if you run the Ingester with SPAN_STORAGE_TYPE=elasticsearch,kafka, the Ingester crash with the following:

WARNING: multiple span storage types have been specified. Only the first type (elasticsearch) will be used for reading and archiving.

flag redefined: kafka.brokers
panic: flag redefined: kafka.brokers

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc000080d80, 0xefd9a0, 0xc00018d080, 0xe04f6e, 0xd, 0xe2cb58, 0x4b)
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/flag/flag.go:805 +0x529
flag.(*FlagSet).StringVar(0xc000080d80, 0xc00018d080, 0xe04f6e, 0xd, 0xe05426, 0xe, 0xe2cb58, 0x4b)
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/flag/flag.go:708 +0x8a
flag.(*FlagSet).String(0xc000080d80, 0xe04f6e, 0xd, 0xe05426, 0xe, 0xe2cb58, 0x4b, 0xe00e66)
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/flag/flag.go:721 +0x8b
github.com/jaegertracing/jaeger/cmd/ingester/app.AddFlags(0xc000080d80)
	/home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/ingester/app/flags.go:84 +0x6e
github.com/jaegertracing/jaeger/pkg/config.AddFlags(0xc00015a0f0, 0xc000186240, 0xc000159f28, 0x5, 0x5, 0xc00003a042, 0xd)
	/home/travis/gopath/src/github.com/jaegertracing/jaeger/pkg/config/config.go:36 +0x56
main.main()
	/home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/ingester/main.go:138 +0x59f

I'm not sure how, but I feel we should add a validation on the storage type to prevent the use to KAFKA in the Ingester. Unless we want to support Kafka -> Kafka with the Ingester, but that feels wrong

ledor473 requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners September 28, 2018 17:47

yurishkuro approved these changes Sep 28, 2018

View reviewed changes

ledor473 force-pushed the ingester branch from 91fe359 to fecd429 Compare September 28, 2018 18:00

ledor473 force-pushed the ingester branch from cce0759 to de69b5a Compare October 22, 2018 17:24

ledor473 commented Oct 22, 2018

View reviewed changes

ledor473 force-pushed the ingester branch from de69b5a to 345a04f Compare October 30, 2018 14:04

yurishkuro changed the title ~~Ingester improvements~~ [ingester] Add metrics& healthcheck, rename Kafka cli flags Oct 30, 2018

yurishkuro requested a review from tiffon as a code owner October 30, 2018 18:54

ghost assigned yurishkuro Oct 30, 2018

ghost added the review label Oct 30, 2018

yurishkuro requested a review from objectiser as a code owner October 31, 2018 22:11

objectiser reviewed Nov 1, 2018

View reviewed changes

pavolloffay mentioned this pull request Nov 1, 2018

[tracking] Release 1.8 #1150

Closed

7 tasks

ledor473 force-pushed the ingester branch from 48b8792 to 5155cbd Compare November 1, 2018 17:37

Louis-Etienne Dorval added 3 commits November 1, 2018 15:24

Ingester: add default healthcheck port, add a configurable metrics ht…

bf6d029

…tp handler and make Kafka flags the same as in the Collector Signed-off-by: Louis-Etienne Dorval <[email protected]>

Move encoding under kafka and fix tests

1b4f0be

Signed-off-by: Louis-Etienne Dorval <[email protected]>

Fixed redefined flag

35d3f1b

Signed-off-by: Louis-Etienne Dorval <[email protected]>

Fix comment on the KafkaConfigPrefix

f466b0a

Signed-off-by: Louis-Etienne Dorval <[email protected]>

ledor473 force-pushed the ingester branch from 5155cbd to f466b0a Compare November 1, 2018 19:24

pavolloffay approved these changes Nov 2, 2018

View reviewed changes

pavolloffay merged commit 44a969d into jaegertracing:master Nov 2, 2018

ghost removed the review label Nov 2, 2018

yurishkuro added this to the 1.8 Release milestone Nov 9, 2018

yurishkuro mentioned this pull request Nov 15, 2018

[ingester] Crash if Kafka is specified as storage #1190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ingester] Add metrics& healthcheck, rename Kafka cli flags #1094

[ingester] Add metrics& healthcheck, rename Kafka cli flags #1094

ledor473 commented Sep 28, 2018 •

edited

Loading

yurishkuro left a comment

yurishkuro Sep 28, 2018

yurishkuro Sep 28, 2018

ledor473 commented Oct 3, 2018

yurishkuro commented Oct 22, 2018

ledor473 Oct 22, 2018

yurishkuro Oct 30, 2018 •

edited

Loading

ledor473 Nov 1, 2018

codecov bot commented Oct 22, 2018 •

edited

Loading

jpkrohling commented Oct 30, 2018

ledor473 commented Oct 30, 2018

jpkrohling commented Oct 30, 2018

yurishkuro commented Oct 30, 2018

objectiser Nov 1, 2018

ledor473 Nov 1, 2018

objectiser Nov 1, 2018

ledor473 Nov 1, 2018

pavolloffay commented Nov 1, 2018

yurishkuro commented Nov 1, 2018

ledor473 commented Nov 1, 2018

pavolloffay commented Nov 2, 2018

ledor473 commented Nov 15, 2018 •

edited

Loading

		@@ -33,6 +33,8 @@ const (

		// ConfigPrefix is a prefix fro the ingester flags

		ConfigPrefix = "ingester"
		// KafkaConfigPrefix is a prefix for the ingester flags

[ingester] Add metrics& healthcheck, rename Kafka cli flags #1094

[ingester] Add metrics& healthcheck, rename Kafka cli flags #1094

Conversation

ledor473 commented Sep 28, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

Comments:

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledor473 commented Oct 3, 2018

yurishkuro commented Oct 22, 2018

Choose a reason for hiding this comment

yurishkuro Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 22, 2018 • edited Loading

Codecov Report

jpkrohling commented Oct 30, 2018

ledor473 commented Oct 30, 2018

jpkrohling commented Oct 30, 2018

yurishkuro commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Nov 1, 2018

yurishkuro commented Nov 1, 2018

ledor473 commented Nov 1, 2018

pavolloffay commented Nov 2, 2018

ledor473 commented Nov 15, 2018 • edited Loading

ledor473 commented Sep 28, 2018 •

edited

Loading

yurishkuro Oct 30, 2018 •

edited

Loading

codecov bot commented Oct 22, 2018 •

edited

Loading

ledor473 commented Nov 15, 2018 •

edited

Loading