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 6 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
19 changes: 8 additions & 11 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,14 @@ 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"
// CollectorDefaultHealthCheckHTTPPort is the default HTTP Port for health check
CollectorDefaultHealthCheckHTTPPort = 14269
)

// CollectorOptions holds configuration for collector
Expand All @@ -44,8 +45,6 @@ type CollectorOptions struct {
CollectorHTTPPort int
// CollectorZipkinHTTPPort is the port that the Zipkin collector service listens in on for http requests
CollectorZipkinHTTPPort int
// CollectorHealthCheckHTTPPort is the port that the health check service listens in on for http requests
CollectorHealthCheckHTTPPort int
}

// AddFlags adds flags for CollectorOptions
Expand All @@ -55,7 +54,6 @@ 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")
}

// InitFromViper initializes CollectorOptions with properties from viper
Expand All @@ -65,6 +63,5 @@ 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)
return cOpts
}
8 changes: 3 additions & 5 deletions cmd/collector/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,13 @@ func main() {
if err != nil {
return err
}

builderOpts := new(builder.CollectorOptions).InitFromViper(v)
hc, err := healthcheck.
New(healthcheck.Unavailable, healthcheck.Logger(logger)).
Serve(builderOpts.CollectorHealthCheckHTTPPort)
hc, err := sFlags.NewHealthCheck(logger, builder.CollectorDefaultHealthCheckHTTPPort)
if err != nil {
logger.Fatal("Could not start the health check server.", zap.Error(err))
}

builderOpts := new(builder.CollectorOptions).InitFromViper(v)

mBldr := new(pMetrics.Builder).InitFromViper(v)
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger-collector")
if err != nil {
Expand Down
26 changes: 23 additions & 3 deletions cmd/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ import (
"go.uber.org/zap"
"go.uber.org/zap/zapcore"

hc "github.com/jaegertracing/jaeger/pkg/healthcheck"
"github.com/jaegertracing/jaeger/plugin/storage"
)

const (
spanStorageType = "span-storage.type" // deprecated
logLevel = "log-level"
configFile = "config-file"
spanStorageType = "span-storage.type" // deprecated
logLevel = "log-level"
configFile = "config-file"
healthCheckHTTPPort = "health-check-http-port"
)

// AddConfigFileFlag adds flags for ExternalConfFlags
Expand All @@ -53,21 +55,29 @@ func TryLoadConfigFile(v *viper.Viper) error {
type SharedFlags struct {
// Logging holds logging configuration
Logging logging
// HealthCheck holds health check configuration
HealthCheck healthCheck
}

type logging struct {
Level string
}

type healthCheck struct {
Port int
}

// 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")
Copy link
Copy Markdown
Contributor Author

@eundoosong eundoosong Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

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.

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)

}

// InitFromViper initializes SharedFlags with properties from viper
func (flags *SharedFlags) InitFromViper(v *viper.Viper) *SharedFlags {
flags.Logging.Level = v.GetString(logLevel)
flags.HealthCheck.Port = v.GetInt(healthCheckHTTPPort)
return flags
}

Expand All @@ -81,3 +91,13 @@ func (flags *SharedFlags) NewLogger(conf zap.Config, options ...zap.Option) (*za
conf.Level = zap.NewAtomicLevelAt(level)
return conf.Build(options...)
}

// NewHealthCheck returns health check based on configuration in SharedFlags
func (flags *SharedFlags) NewHealthCheck(logger *zap.Logger, defaultPort int) (*hc.HealthCheck, error) {
port := defaultPort
if flags.HealthCheck.Port > 0 {
port = flags.HealthCheck.Port
}
return hc.New(hc.Unavailable, hc.Logger(logger)).
Serve(port)
}
15 changes: 6 additions & 9 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"
// QueryDefaultHealthCheckHTTPPort is the default HTTP Port for health check
QueryDefaultHealthCheckHTTPPort = 16687
)

// QueryOptions holds configuration for query service
Expand All @@ -38,8 +39,6 @@ type QueryOptions struct {
StaticAssets string
// UIConfig is the path to a configuration file for the UI
UIConfig string
// HealthCheckHTTPPort is the port that the health check service listens in on for http requests
HealthCheckHTTPPort int
}

// AddFlags adds flags for QueryOptions
Expand All @@ -48,7 +47,6 @@ 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")
}

// InitFromViper initializes QueryOptions with properties from viper
Expand All @@ -57,6 +55,5 @@ 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)
return qOpts
}
8 changes: 3 additions & 5 deletions cmd/query/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,13 @@ func main() {
if err != nil {
return err
}

queryOpts := new(app.QueryOptions).InitFromViper(v)
hc, err := healthcheck.
New(healthcheck.Unavailable, healthcheck.Logger(logger)).
Serve(queryOpts.HealthCheckHTTPPort)
hc, err := sFlags.NewHealthCheck(logger, app.QueryDefaultHealthCheckHTTPPort)
if err != nil {
logger.Fatal("Could not start the health check server.", zap.Error(err))
}

queryOpts := new(app.QueryOptions).InitFromViper(v)

mBldr := new(pMetrics.Builder).InitFromViper(v)
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger-query")
if err != nil {
Expand Down
17 changes: 14 additions & 3 deletions cmd/standalone/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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 +57,8 @@ import (
zc "github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
)

const defaultHealthCheckPort = collector.CollectorDefaultHealthCheckHTTPPort

// 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 @@ -89,6 +92,10 @@ func main() {
if err != nil {
return err
}
hc, err := sFlags.NewHealthCheck(logger, defaultHealthCheckPort)
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")
Expand Down Expand Up @@ -119,8 +126,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 Down Expand Up @@ -180,6 +188,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 +231,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 +261,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 +299,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