Add healthcheck to standalone #784

eundoosong · 2018-04-23T15:25:30Z

healthcheck is added in the same way as collect and query since it is missed in standalone.
Related issue : #775

Signed-off-by: Eundoo Song [email protected]

Signed-off-by: Eundoo Song <[email protected]>

coveralls · 2018-04-23T16:03:56Z

Coverage remained the same at 100.0% when pulling af74207 on eundoosong:healcheck_standalone into 5eb2922 on jaegertracing:master.

yurishkuro · 2018-04-23T17:39:30Z

So this is actually adding two independent health check endpoints, for collector and query.

Could you describe in the PR why this is needed, how it can be used? Our objective is to only add features that are actually useful to people (and documented!).

Thanks.

eundoosong · 2018-04-23T23:13:25Z

I think it would be better for people who want to use health check in the same way as separated one.
Because it is more consistent rather than providing another end point.
Plus, help message already shows each healthcheck flags of collector and query.
Documentation should be as different PR in documentation repo.

Thanks.

yurishkuro · 2018-04-23T23:31:26Z

I think it would be better for people who want to use health check in the same way as separated one.

But how would you use it? E.g. our internal deployment system and discovery system (part of routing) only support one health endpoint per service.

I am not suggesting a separate endpoint (we serve metrics from one port only, healthcheck would be on the same port), but I think there should be only one health status for the binary, not two that are potentially conflicting.

jpkrohling · 2018-04-24T08:51:55Z

I think there should be only one health status for the binary, not two that are potentially conflicting.

+1, from an operator's perspective, it makes more sense to have one endpoint that reports the health of the system in general than one health check endpoint for each component.

eundoosong · 2018-04-24T15:57:53Z

we serve metrics from one port only, healthcheck would be on the same port

I don't get this, does this mean healthcheck is using the same port as metrics?

I agree that one endpoint of the health check per service(binary) makes more sense.
Thanks for pointing out.
So, if you agree with adding one healthcheck in standalone, the following should be considered.

Add a healthcheck for standalone. (with new port number)
Add a flag like "standalone.health-check-http-port"
Hide existing collector(--collector.health-check-http-port) and query(--query.health-check-http-port) flags

I think the implementation will not be many changes, but I'm wondering which port number can be used.

yurishkuro · 2018-04-24T16:10:53Z

You don't need a new port, all-in-one already opens several HTTP ports, you can just add an endpoint to one of them. E.g. we define ports for the /metrics endpoint here https://www.jaegertracing.io/docs/monitoring/. In all-in-one the /metrics endpoint is served from query service API port (https://github.com/jaegertracing/jaeger/pull/784/files#diff-053312057ab6b9282b572674adc5b7f6R298), so it makes the most sense to me to add the /health endpoint to the same port.

…jaeger into healcheck_standalone

Signed-off-by: eundoo-song <[email protected]>

eundoosong · 2018-04-26T06:00:02Z

Your suggestion looks good. But since health check is using its own HTTP server(with ServeMux) and the other service(collector, query) is using Gorilla Mux, it's a bit difficult for health check to use other Mux without change in health check. I did implement (eundoosong@f6a8f5d) but IMO it doesn't look better in term of design.

So I decided collector port for health check(port num is 14269) is used for standalone health check port.
Please review.

eundoosong · 2018-04-26T06:06:25Z

cmd/standalone/main.go

+		func(flagSet *flag.FlagSet) {
+			flagSet.Int(healthCheckHTTPPort, defaultHealthCheckPort,
+				"The http port for the health check services")
+		},


health-check for standalone is implemented inside main because there is only one flag existed in standalone.

eundoosong · 2018-04-26T06:11:51Z

cmd/standalone/main.go


 	if err := command.Execute(); err != nil {
 		fmt.Println(err.Error())
 		os.Exit(1)
 	}
 }

+func hideQueryCollectorFlags(command *cobra.Command) {


This hides query and collector health check flags.

yurishkuro · 2018-04-26T15:45:10Z

cmd/collector/app/builder/builder_flags.go

+	collectorHTTPPort      = "collector.http-port"
+	collectorZipkinHTTPort = "collector.zipkin.http-port"
+	// CollectorHealthCheckHTTPPort is the flag name for health check
+	CollectorHealthCheckHTTPPort = "collector.health-check-http-port"


I think a better approach would be to have a single global flag for all binaries, similar to how we do log levels or metrics.

Questions,

Does this mean we need to take that flag out from collector flags and make a global flag?

similar to how we do log levels or metrics.

Are you talking about BasicOptions(cmd/builder/builder_options.go)? How about SharedFlags(cmd/flags/..)?

1 - yes
2 - I meant shared flags, the BasicOption is independent of cli flags

Signed-off-by: Eundoo Song <[email protected]>

eundoosong · 2018-04-30T00:53:13Z

cmd/flags/flags.go

 // AddFlags adds flags for SharedFlags
 func AddFlags(flagSet *flag.FlagSet) {
 	flagSet.String(spanStorageType, "", fmt.Sprintf("Deprecated; please use %s environment variable", storage.SpanStorageTypeEnvVar))
 	flagSet.String(logLevel, "info", "Minimal allowed log Level. For more levels see https://github.com/uber-go/zap")
+	flagSet.Int(healthCheckHTTPPort, 0, "The http port for the health check service")


The default port is removed since it's difficult to pass in the current interface. So I let NewHealthCheck set it when each component initializes health check.
Do we need to add a default port in help message? Maybe like below..

flagSet.Int(healthCheckHTTPPort, 0, "The http port for the health check service. For default port, see https://https://www.jaegertracing.io/XXXXX")

It would be better of the default port was printed by help. One option to do that is to define it in a global variable, e.g. defaulted to collector port, and then query service main() can override that var at the beginning (before registering flags)

Signed-off-by: Eundoo Song <[email protected]>

yurishkuro · 2018-04-30T17:34:20Z

cmd/standalone/main.go

-			startQuery(qOpts, spanReader, dependencyReader, logger, metricsFactory, mBldr)
+			startCollector(cOpts, spanWriter, logger, metricsFactory, samplingHandler, hc)
+			startQuery(qOpts, spanReader, dependencyReader, logger, metricsFactory, mBldr, hc)
+			hc.Ready()


I don't necessarily know how to fix it, but there is a race condition here where collector or query background goroutines can fail to start the server and set HC to Unavailable, but this statement will override it.

yurishkuro

aside from race issue in all-in-one, lgtm

eundoosong · 2018-05-09T16:07:38Z

cmd/standalone/main.go

+			logger.Info("Starting jaeger-query HTTP server", zap.Int("port", qOpts.Port))
+			go collectorServer()
+			go queryServer()
+			hc.Ready()


Move goroutines to main from each start function, and start them in main then set Ready.
IMO, this will prevent the race issue.

For 100% guarantees, can we move hc.Ready before goroutines like this?

hc.Ready() go collectorServer() go queryServer()

I don't think returning functions like this provides any additional guarantees. I think it's fine to leave it as it was, i.e. call startCollector(), it might fail mid-way and crash the program, or it can spawn goroutine for ListenAndServe, which can fail as well and crush the program later. So there will be a possible time interval where HC==Ready, and then a crash, but I don't think it's critical for all-in-one.

Sorry I led you on the wrong path with the last comment. The fact that the goroutines call log.Fatal on error sort of take care of things.

I wanted hc.Ready() to be called before collector/query goroutine call hc.Set(healthcheck.Unavailable) in ListenAndServe error.
I think this would prevent overriding Unavailable by hc.Ready() as you mentioned above coment.

Anyway, if the issue in all-in-one is not critical and fatal log on error is enough, I'll revert.

Yes, I don't think it's critical sice even if the override happens the process will exit anyway.

eundoosong · 2018-05-11T02:15:41Z

Reverted.
Ready to be merged, or is there anything else missing?

yurishkuro

Thanks!

* master: (38 commits) Preparing release 1.5.0 (jaegertracing#847) Add bounds to memory storage (jaegertracing#845) Add metric for debug traces (jaegertracing#796) Change metrics naming scheme (jaegertracing#776) Bump gocql version (jaegertracing#829) Remove ParentSpanID from domain model (jaegertracing#831) Make gas run quiet (jaegertracing#838) Revert "Make gas run quite" Revert "Install gas from install-ci" Install gas from install-ci Make gas run quite Add 'gas' for security problems scanning (jaegertracing#830) Add ability to adjust static sampling probabilities per operation (jaegertracing#827) Support log-level flag on agent (jaegertracing#828) Remove unused function (jaegertracing#822) Add healthcheck to standalone (jaegertracing#784) Do not use KeyValue fields directly and use KeyValues as decorator only (jaegertracing#810) Add ContaAzul to the adopters list (jaegertracing#806) Add ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE (jaegertracing#805) Upgrade to go 1.10 (jaegertracing#792) ... # Conflicts: # cmd/agent/app/builder.go # cmd/collector/main.go # cmd/query/main.go # cmd/standalone/main.go

eundoosong added 2 commits April 19, 2018 01:14

add healthcheck to standalone

5673f8b

Signed-off-by: Eundoo Song <[email protected]>

Add healthcheck to standalone

111c618

Signed-off-by: Eundoo Song <[email protected]>

eundoosong requested review from black-adder, pavolloffay, vprithvi and yurishkuro as code owners April 23, 2018 15:25

Fix import ordering

fa7acd8

Signed-off-by: Eundoo Song <[email protected]>

eundoo-song added 2 commits April 26, 2018 11:18

Merge branch 'healcheck_standalone' of https://github.com/eundoosong/…

fe389b7

…jaeger into healcheck_standalone

Add one health check for standalone

1e41f84

Signed-off-by: eundoo-song <[email protected]>

eundoosong commented Apr 26, 2018

View reviewed changes

yurishkuro reviewed Apr 26, 2018

View reviewed changes

Add healthcheck in SharedFlags

0a9e445

Signed-off-by: Eundoo Song <[email protected]>

eundoosong commented Apr 30, 2018

View reviewed changes

Add SetDefaultHealthCheckPort to set healthcheck port

31cac87

Signed-off-by: Eundoo Song <[email protected]>

yurishkuro reviewed Apr 30, 2018

View reviewed changes

Merge branch 'master' into healcheck_standalone

466f7b7

ghost assigned yurishkuro Apr 30, 2018

ghost added the review label Apr 30, 2018

eundoosong force-pushed the healcheck_standalone branch from a0d42ad to 64b9368 Compare May 9, 2018 15:45

eundoosong commented May 9, 2018

View reviewed changes

eundoosong mentioned this pull request May 10, 2018

Jaeger should use healthcheck for readinessProbe jaegertracing/jaeger-kubernetes#86

Closed

eundoosong force-pushed the healcheck_standalone branch from 64b9368 to 466f7b7 Compare May 10, 2018 14:45

Merge branch 'master' into healcheck_standalone

af74207

yurishkuro approved these changes May 11, 2018

View reviewed changes

yurishkuro merged commit a4706c0 into jaegertracing:master May 11, 2018

ghost removed the review label May 11, 2018

ledor473 mentioned this pull request May 23, 2018

Add default healthcheck port in the documentation jaegertracing/documentation#80

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add healthcheck to standalone #784

Add healthcheck to standalone #784

eundoosong commented Apr 23, 2018 •

edited

Loading

coveralls commented Apr 23, 2018 •

edited

Loading

yurishkuro commented Apr 23, 2018

eundoosong commented Apr 23, 2018

yurishkuro commented Apr 23, 2018

jpkrohling commented Apr 24, 2018

eundoosong commented Apr 24, 2018 •

edited

Loading

yurishkuro commented Apr 24, 2018

eundoosong commented Apr 26, 2018 •

edited

Loading

eundoosong Apr 26, 2018 •

edited

Loading

eundoosong Apr 26, 2018

yurishkuro Apr 26, 2018

eundoosong Apr 26, 2018

yurishkuro Apr 26, 2018

eundoosong Apr 30, 2018 •

edited

Loading

yurishkuro Apr 30, 2018

yurishkuro Apr 30, 2018

yurishkuro left a comment

eundoosong May 9, 2018

yurishkuro May 9, 2018

eundoosong May 10, 2018

yurishkuro May 10, 2018

eundoosong commented May 11, 2018

yurishkuro left a comment

Add healthcheck to standalone #784

Add healthcheck to standalone #784

Conversation

eundoosong commented Apr 23, 2018 • edited Loading

coveralls commented Apr 23, 2018 • edited Loading

yurishkuro commented Apr 23, 2018

eundoosong commented Apr 23, 2018

yurishkuro commented Apr 23, 2018

jpkrohling commented Apr 24, 2018

eundoosong commented Apr 24, 2018 • edited Loading

yurishkuro commented Apr 24, 2018

eundoosong commented Apr 26, 2018 • edited Loading

eundoosong Apr 26, 2018 • edited Loading

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

Choose a reason for hiding this comment

eundoosong Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eundoosong commented May 11, 2018

yurishkuro left a comment

Choose a reason for hiding this comment

eundoosong commented Apr 23, 2018 •

edited

Loading

coveralls commented Apr 23, 2018 •

edited

Loading

eundoosong commented Apr 24, 2018 •

edited

Loading

eundoosong commented Apr 26, 2018 •

edited

Loading

eundoosong Apr 26, 2018 •

edited

Loading

eundoosong Apr 30, 2018 •

edited

Loading