Skip to content

Add healthcheck to standalone #784

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

Merged
yurishkuro merged 9 commits into from
May 11, 2018
Merged

Add healthcheck to standalone #784

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5673f8b
add healthcheck to standalone
eundoosong Apr 18, 2018
111c618
Add healthcheck to standalone
eundoosong Apr 18, 2018
fa7acd8
Fix import ordering
eundoosong Apr 23, 2018
fe389b7
Merge branch 'healcheck_standalone' of https://github.com/eundoosong/…
Apr 26, 2018
1e41f84
Add one health check for standalone
Apr 26, 2018
0a9e445
Add healthcheck in SharedFlags
eundoosong Apr 27, 2018
31cac87
Add SetDefaultHealthCheckPort to set healthcheck port
eundoosong Apr 30, 2018
466f7b7
Merge branch 'master' into healcheck_standalone
yurishkuro Apr 30, 2018
af74207
Merge branch 'master' into healcheck_standalone
yurishkuro May 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions cmd/collector/app/builder/builder_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@ import (
)

const (
collectorQueueSize = "collector.queue-size"
collectorNumWorkers = "collector.num-workers"
collectorWriteCacheTTL = "collector.write-cache-ttl"
collectorPort = "collector.port"
collectorHTTPPort = "collector.http-port"
collectorZipkinHTTPort = "collector.zipkin.http-port"
collectorHealthCheckHTTPPort = "collector.health-check-http-port"
collectorQueueSize = "collector.queue-size"
collectorNumWorkers = "collector.num-workers"
collectorWriteCacheTTL = "collector.write-cache-ttl"
collectorPort = "collector.port"
collectorHTTPPort = "collector.http-port"
collectorZipkinHTTPort = "collector.zipkin.http-port"
// CollectorHealthCheckHTTPPort is the flag name for health check
CollectorHealthCheckHTTPPort = "collector.health-check-http-port"
Copy link
Copy Markdown
Member

@yurishkuro yurishkuro Apr 26, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@eundoosong eundoosong Apr 26, 2018

Choose a reason for hiding this comment

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

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/..)?

Copy link
Copy Markdown
Member

@yurishkuro yurishkuro Apr 26, 2018

Choose a reason for hiding this comment

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

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

// DefaultHealthCheckHTTPPort is the default HTTP Port for health check
DefaultHealthCheckHTTPPort = 14269
)

// CollectorOptions holds configuration for collector
Expand All @@ -55,7 +58,7 @@ func AddFlags(flags *flag.FlagSet) {
flags.Int(collectorPort, 14267, "The tchannel port for the collector service")
flags.Int(collectorHTTPPort, 14268, "The http port for the collector service")
flags.Int(collectorZipkinHTTPort, 0, "The http port for the Zipkin collector service e.g. 9411")
flags.Int(collectorHealthCheckHTTPPort, 14269, "The http port for the health check service")
flags.Int(CollectorHealthCheckHTTPPort, DefaultHealthCheckHTTPPort, "The http port for the health check service")
}

// InitFromViper initializes CollectorOptions with properties from viper
Expand All @@ -65,6 +68,6 @@ func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) *CollectorOptions {
cOpts.CollectorPort = v.GetInt(collectorPort)
cOpts.CollectorHTTPPort = v.GetInt(collectorHTTPPort)
cOpts.CollectorZipkinHTTPPort = v.GetInt(collectorZipkinHTTPort)
cOpts.CollectorHealthCheckHTTPPort = v.GetInt(collectorHealthCheckHTTPPort)
cOpts.CollectorHealthCheckHTTPPort = v.GetInt(CollectorHealthCheckHTTPPort)
return cOpts
}
15 changes: 8 additions & 7 deletions cmd/query/app/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@ import (
)

const (
queryPort = "query.port"
queryBasePath = "query.base-path"
queryStaticFiles = "query.static-files"
queryUIConfig = "query.ui-config"
queryHealthCheckHTTPPort = "query.health-check-http-port"
queryPort = "query.port"
queryBasePath = "query.base-path"
queryStaticFiles = "query.static-files"
queryUIConfig = "query.ui-config"
// QueryHealthCheckHTTPPort is the flag name for health check
QueryHealthCheckHTTPPort = "query.health-check-http-port"
)

// QueryOptions holds configuration for query service
Expand All @@ -48,7 +49,7 @@ func AddFlags(flagSet *flag.FlagSet) {
flagSet.String(queryBasePath, "/", "The base path for all HTTP routes, e.g. /jaeger; useful when running behind a reverse proxy")
flagSet.String(queryStaticFiles, "jaeger-ui-build/build/", "The directory path for the static assets for the UI")
flagSet.String(queryUIConfig, "", "The path to the UI configuration file in JSON format")
flagSet.Int(queryHealthCheckHTTPPort, 16687, "The http port for the health check service")
flagSet.Int(QueryHealthCheckHTTPPort, 16687, "The http port for the health check service")
}

// InitFromViper initializes QueryOptions with properties from viper
Expand All @@ -57,6 +58,6 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper) *QueryOptions {
qOpts.BasePath = v.GetString(queryBasePath)
qOpts.StaticAssets = v.GetString(queryStaticFiles)
qOpts.UIConfig = v.GetString(queryUIConfig)
qOpts.HealthCheckHTTPPort = v.GetInt(queryHealthCheckHTTPPort)
qOpts.HealthCheckHTTPPort = v.GetInt(QueryHealthCheckHTTPPort)
return qOpts
}
38 changes: 35 additions & 3 deletions cmd/standalone/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package main

import (
"flag"
"fmt"
"log"
"net"
Expand Down Expand Up @@ -44,6 +45,7 @@ import (
"github.com/jaegertracing/jaeger/cmd/flags"
queryApp "github.com/jaegertracing/jaeger/cmd/query/app"
"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/healthcheck"
pMetrics "github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/pkg/recoveryhandler"
"github.com/jaegertracing/jaeger/pkg/version"
Expand All @@ -56,6 +58,11 @@ import (
zc "github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
)

const (
healthCheckHTTPPort = "standalone.health-check-http-port"
defaultHealthCheckPort = collector.DefaultHealthCheckHTTPPort
)

// standalone/main is a standalone full-stack jaeger backend, backed by a memory store
func main() {
var signalsChannel = make(chan os.Signal, 0)
Expand Down Expand Up @@ -90,6 +97,13 @@ func main() {
return err
}

hc, err := healthcheck.
New(healthcheck.Unavailable, healthcheck.Logger(logger)).
Serve(v.GetInt(healthCheckHTTPPort))
if err != nil {
logger.Fatal("Could not start the health check server.", zap.Error(err))
}

mBldr := new(pMetrics.Builder).InitFromViper(v)
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger-standalone")
if err != nil {
Expand Down Expand Up @@ -119,8 +133,9 @@ func main() {
qOpts := new(queryApp.QueryOptions).InitFromViper(v)

startAgent(aOpts, cOpts, logger, metricsFactory)
startCollector(cOpts, spanWriter, logger, metricsFactory, samplingHandler)
startQuery(qOpts, spanReader, dependencyReader, logger, metricsFactory, mBldr)
startCollector(cOpts, spanWriter, logger, metricsFactory, samplingHandler, hc)
startQuery(qOpts, spanReader, dependencyReader, logger, metricsFactory, mBldr, hc)
hc.Ready()
Copy link
Copy Markdown
Member

@yurishkuro yurishkuro Apr 30, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@eundoosong eundoosong May 9, 2018

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

@yurishkuro yurishkuro May 9, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@eundoosong eundoosong May 10, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@yurishkuro yurishkuro May 10, 2018

Choose a reason for hiding this comment

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

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


select {
case <-signalsChannel:
Expand All @@ -144,14 +159,28 @@ func main() {
queryApp.AddFlags,
pMetrics.AddFlags,
strategyStoreFactory.AddFlags,
func(flagSet *flag.FlagSet) {
flagSet.Int(healthCheckHTTPPort, defaultHealthCheckPort,
"The http port for the health check services")
},
Copy link
Copy Markdown
Contributor Author

@eundoosong eundoosong Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

)
hideQueryCollectorFlags(command)

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

func hideQueryCollectorFlags(command *cobra.Command) {
Copy link
Copy Markdown
Contributor Author

@eundoosong eundoosong Apr 26, 2018

Choose a reason for hiding this comment

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

This hides query and collector health check flags.

if err := command.Flags().MarkHidden(queryApp.QueryHealthCheckHTTPPort); err != nil {
fmt.Println(err.Error())
}
if err := command.Flags().MarkHidden(collector.CollectorHealthCheckHTTPPort); err != nil {
fmt.Println(err.Error())
}
}

func startAgent(
b *agentApp.Builder,
cOpts *collector.CollectorOptions,
Expand Down Expand Up @@ -180,6 +209,7 @@ func startCollector(
logger *zap.Logger,
baseFactory metrics.Factory,
samplingHandler sampling.Handler,
hc *healthcheck.HealthCheck,
) {
metricsFactory := baseFactory.Namespace("jaeger-collector", nil)

Expand Down Expand Up @@ -222,6 +252,7 @@ func startCollector(
if err := http.ListenAndServe(httpPortStr, recoveryHandler(r)); err != nil {
logger.Fatal("Could not launch jaeger-collector HTTP server", zap.Error(err))
}
hc.Set(healthcheck.Unavailable)
}()
}

Expand Down Expand Up @@ -251,8 +282,8 @@ func startQuery(
logger *zap.Logger,
baseFactory metrics.Factory,
metricsBuilder *pMetrics.Builder,
hc *healthcheck.HealthCheck,
) {

tracer, closer, err := jaegerClientConfig.Configuration{
Sampler: &jaegerClientConfig.SamplerConfig{
Type: "const",
Expand Down Expand Up @@ -289,6 +320,7 @@ func startQuery(
if err := http.ListenAndServe(portStr, recoveryHandler(r)); err != nil {
logger.Fatal("Could not launch jaeger-query service", zap.Error(err))
}
hc.Set(healthcheck.Unavailable)
}()
}

Expand Down