Add support for client configuration via env vars #275

jpkrohling · 2018-03-27T14:07:35Z

Status: I'm opening this PR mostly to get an early feedback on the general direction. I have the feeling that my Java background might be getting in the way, so, I wanted to catch any fundamental problem early on :)

As it is, this PR implements the configuration of the following properties, like from the Java Client:

Additionally, these are the properties that are unique to the Go client:

JAEGER_DISABLED
JAEGER_RPC_METRICS
JAEGER_SAMPLER_MAX_OPERATIONS
JAEGER_SAMPLER_REFRESH_INTERVAL

The following properties will not be implemented, as the underlying feature is not available for the Go client:

The following property will not be implemented, as the underlying feature behaves differently in the Go client than in the Java client:

~~JAEGER_DISABLE_GLOBAL_TRACER~~

Closes #206

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

codecov · 2018-03-27T14:12:38Z

Codecov Report

Merging #275 into master will increase coverage by 0.39%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   85.74%   86.14%   +0.39%     
==========================================
  Files          54       55       +1     
  Lines        2856     2952      +96     
==========================================
+ Hits         2449     2543      +94     
- Misses        287      288       +1     
- Partials      120      121       +1

Impacted Files	Coverage Δ
transport_udp.go	`92.59% <0%> (ø)`	⬆️
config/config_env.go	`100% <100%> (ø)`
config/config.go	`92.91% <81.81%> (-1.31%)`	⬇️

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 3516374...2dd39a3. Read the comment docs.

pavolloffay · 2018-03-27T14:44:10Z

config/config_test.go

@@ -70,6 +71,77 @@ func TestDefaultSampler(t *testing.T) {
 	require.Error(t, err)
 }

+func TestServiceNameFromEnv(t *testing.T) {
+	os.Setenv("JAEGER_SERVICE_NAME", "my-service")


Shouldn't we use local constants for these ENV names?

In Java, yes. In Go? I don't know... I have a feeling that creating a constant JAEGER_SERVICE_NAME with the value JAEGER_SERVICE_NAME is not idiomatic

The constant name can be different, go idiomatic. I am asking for it because there is some repetition - code, tests, logs, comments

yes, typically we try to avoid string constants directly in the code. Having them all in a single place as constants makes it easier to survey and document

pavolloffay · 2018-03-27T14:59:31Z

config/config.go

@@ -207,6 +226,55 @@ func (c Configuration) New(
 	return tracer, closer, nil
 }

+// FromEnv uses environment variables to set the tracer's Configuration
+func (c *Configuration) FromEnv() (*Configuration, error) {


I am wondering whether:

func FromEnv() (*Configuration, error) {

would be better/cleaner. Then we don't have to care about overrides. It would also align more with java configuration (static fromEnv).

Might be, yes. This means that people can't load the env vars into an existing object. Is that OK?

They can create configuration from ENV and then adjust behaviour in code

And we then have SamplerConfigFromEnv() / ReporterConfigFromEnv() / ...FromEnv() ? Sounds good to me.

pavolloffay · 2018-03-27T15:00:11Z

config/config.go

+}
+
+// NewFromEnv returns a new tracer configured using environment variables
+func (c *Configuration) NewFromEnv(options ...Option) (opentracing.Tracer, io.Closer, error) {


-1 on this method, also fromEnv should return the error if the service name is not valid.

This one exists because it has a different semantic as the other. The idea of FromEnv() methods is to configure an existing object based on env vars. This one will create a new tracer (similar to the New() method), running FromEnv() before.

This means that it's still possible to use the New(serviceName, ...) after calling FromEnv(), so, FromEnv() should not fail if an env var with the service name is not available.

I'm open to change this behavior, though. What do others say?

I think it's redundant having FromEnv() that returns Config and NewFromEnv that returns a tracer, since the latter can be implemented as FromEnv().New()

the existing method called New() is poorly named, imo, it should've been NewTracer()

I think it's redundant having FromEnv() that returns Config and NewFromEnv that returns a tracer, since the latter can be implemented as FromEnv().New()

jaegerconfig.FromEnv().New() would require a change in the signature of New() to remove the service name from it. Or did you mean jaegerconfig.FromEnv().New(string, ...Options)?

It's also not clear if it would be preferrable to have @pavolloffay's approach (jaegerconfig.FromEnv()) or the existing one (&Configuration{}.FromEnv()). I like @pavolloffay's suggestion, but I like consistency more :)

the existing method called New() is poorly named, imo, it should've been NewTracer()

Should I create a NewTracer(), and make the existing New() call it? How would deprecation work here?

no I wasn't suggesting changing the signature of New(), we should keep it. Go doesn't have an official way of deprecating packages afaik, but the convention is to add the last comment line as Deprecated: explanation

On static func vs. member func, I don't have a strong opinion - preferably we should be consistent with other clients.

since the latter can be implemented as FromEnv().New()

This is the part that I'm not quite following. New(string, ...Option) requires a service name as the first parameter. Using FromEnv(), I assume we'd be getting the service name from the env var, but we would still require the caller to pass a service name.

One solution is to allow the caller to pass an empty value, but that doesn't sound that clean to me.

Should I just create a NewTracer(...Option), without serviceName?

jpkrohling · 2018-03-28T10:00:39Z

config/config_test.go

+func TestServiceNameFromEnv(t *testing.T) {
+	os.Setenv(envServiceName, "my-service")
+
+	_, c, err := FromEnv().New("")


I don't quite like that callers are required to pass an empty string if they expect the service name to be specified via env var, but perhaps it's acceptable?

as the service name is a member of configuration, we could deprecate New and create newTracer without requiring serviceName.

oh you had the same suggestion here #275 (comment)

I like it

jpkrohling · 2018-03-28T14:08:14Z

This is now ready for the next round of reviews, but please don't merge. I'll squash it into a single commit.

cc @yurishkuro, @pavolloffay

edit: just realized I did not add any documentation about this feature to the readme files... So, please do start the review, but keep in mind I'll work on the readme.

yurishkuro · 2018-03-28T15:10:32Z

config/config.go

 // Configuration configures and creates Jaeger Tracer
 type Configuration struct {
-	Disabled            bool                       `yaml:"disabled"`
+	// ServiceName specifies the service name to use on the tracer.
+	// Can be set by exporting an environment variable named JAEGER_SERVICE_NAME


nit: Can be provided via environment variable named JAEGER_SERVICE_NAME

yurishkuro · 2018-03-28T15:14:10Z

config/config.go

+	// RPCMetrics can be set by exporting an environment variable named JAEGER_RPC_METRICS
+	RPCMetrics bool `yaml:"rpc_metrics"`
+
+	Tags                []opentracing.Tag          `yaml:"tags"`


add comment about env var?

add blank line after

yurishkuro · 2018-03-28T15:17:29Z

config/config.go

+}
+
+// FromEnv uses environment variables to set the tracer's Configuration
+func FromEnv() *Configuration {


nit - this is making the file huge, I suggest moving to config_env.go (including constants) for a nice encapsulation

yurishkuro · 2018-03-28T15:19:42Z

config/config.go

+	if e := os.Getenv(envServiceName); e != "" {
+		// We are ignoring any errors, so that we have a single return value
+		// This pattern repeats itself across all the "FromEnv" functions.
+		// TODO: should we at least log when something bad happens, instead of just ignoring?


I think it's best not to ignore errors. Many env vars are parsed (e.g. as boolean), it's better to fail if we can't parse.

This means then that FromEnv() has a multiple valued return, making it impossible to do FromEnv().NewTracer(), no ?

correct, but it's better than swallowing errors. FromEnv().NewTracer() is not exactly the goal here. If people really want a one-liner, we could add NewTracerFromEnv()

Alright, just wanted to make sure we were on the same page :)

yurishkuro · 2018-03-28T15:20:46Z

config/config.go

+}
+
+// SamplerConfigFromEnv creates a new SamplerConfig based on the environment variables
+func SamplerConfigFromEnv() *SamplerConfig {


nit: how about we keep these private for now? Can always expose later if someone actually needs that

yurishkuro · 2018-03-28T15:21:46Z

config/config.go

+	// spec for this value:
+	// - comma separated list of key=value
+	// - value can be specified using the notation ${envVar:defaultValue}, where `envVar`
+	// is an environment variable and `defaultValue` is the value to use in case the env var is not set


move to method comment

yurishkuro · 2018-03-28T15:25:23Z

config/config.go

+		return nil, nil, errors.New("no service name provided")
+	}
+
+	return c.New(c.ServiceName, options...)


nit: it would be better to do the reverse - implement New via NewTracer

yurishkuro · 2018-03-28T15:28:32Z

config/config_test.go

+	ts := parseTags(tags)
+	require.Equal(t, 4, len(ts))
+
+	require.Equal(t, "key", ts[0].Key)


nit: require means you cannot continue the test further, should be rarely used, in most cases assert is better.

yurishkuro · 2018-03-28T15:30:00Z

config/config_test.go

+func TestNewTracerWithoutServiceName(t *testing.T) {
+	cfg := &Configuration{}
+	_, _, err := cfg.NewTracer(Metrics(metrics.NullFactory), Logger(log.NullLogger))
+	require.Error(t, err)


nit: this test may succeed as false positive. Best to check not only that there was an error, but that the error contained a certain string, e.g. assert.Contains(t, err.Error(), "pattern")

yurishkuro · 2018-03-28T15:41:37Z

Closes #206 - Added support for client configuration via env vars #275

Please follow commit comment guidelines from Contributing.md for PR titles.

In particular, the reference to the ticket being addressed must be in the description, not the title, otherwise github won't close the ticket on merge.

jpkrohling · 2018-03-28T16:24:23Z

I believe all the comments were addressed by this last change.

yurishkuro · 2018-03-28T16:40:37Z

README.md

+JAEGER_SAMPLER_MAX_OPERATIONS | The maximum number of operations that the sampler will keep track of
+JAEGER_SAMPLER_REFRESH_INTERVAL | How often the remotely controlled sampler will poll jaeger-agent for the appropriate sampling strategy
+JAEGER_TAGS | A comma separated list of `name = value` tracer level tags, which get added to all reported spans. The value can also refer to an environment variable using the format `${envVarName:default}`, where the `:default` is optional, and identifies a value to be used if the environment variable cannot be found
+JAEGER_DISABLED | Whether the tracer is disabled or not


might want to clarify: if true, the default opentracing.NoopTracer is used.

yurishkuro · 2018-03-28T16:42:45Z

config/config.go

@@ -115,9 +135,23 @@ func (c Configuration) New(
 	serviceName string,
 	options ...Option,
 ) (opentracing.Tracer, io.Closer, error) {
-	if serviceName == "" {
+	if serviceName == "" && c.ServiceName == "" {


I think this check is not necessary, since it will be done by NewTracer anyway

True, but given that we have ServiceName as part of the Configuration object, this would cover the following scenario:

cfg := &Configuration{ServiceName: "my-service", ...} _, closer, err := cfg.New("")

Without the right side of the check, the ServiceName from the Configuration is ignored and the error is returned ("no service name provided").

I added a new test for this scenario.

not sure I follow. If serviceName arg is not blank, it will override c.ServiceName, otherwise it wouldn't. In either case it will be followed by a call to c.NewTracer() which will return an error if c.ServiceName is blank.

If you call NewTracer(), yes. But New("") might be called directly:

cfg := &Configuration{ServiceName: "my-service", ...} _, closer, err := cfg.New("")

but New() delegates to NewTracer():

return c.NewTracer(options...)

Oh, I see now what you mean. You mean to just entirely remove the first conditional. Agree :)

The diff colors confused me:

yurishkuro · 2018-03-28T16:43:38Z

config/config.go

+	return c.NewTracer(options...)
+}
+
+// NewTracer returns a new tracer based on the current configuration, using the given options


add info about closer function (see comment for New)

yurishkuro · 2018-03-28T16:44:18Z

config/config.go

@@ -115,9 +135,23 @@ func (c Configuration) New(
 	serviceName string,


add // Deprecated: use NewTracer() function as the last line in the comment

yurishkuro · 2018-03-28T16:49:46Z

config/config_env.go

+		if value, err := strconv.ParseInt(e, 10, 0); err == nil {
+			rc.QueueSize = int(value)
+		} else {
+			return nil, err


nit: a better way would be to mention the field, e.g.

return nil, errors.Wraps(err, "Cannot parse env var %s=%s", envReporterMaxQueueSize, e)

yurishkuro · 2018-03-28T16:52:44Z

config/config_env.go

+	// JAEGER_AGENT_HOST:JAEGER_AGENT_PORT, like: localhost:6831
+	if e := os.Getenv(envAgentHost); e != "" {
+		host := e
+		port := 6831 // TODO: can we somehow consume the default from the transport itself?


there is const defaultUDPSpanServerHostPort = "localhost:6831", the port could be refactored into a public const (maybe even moved to constants.go)

The host as well then, for consistency :)

yurishkuro · 2018-03-28T16:53:32Z

config/config_env.go

+	}
+
+	// JAEGER_AGENT_HOST:JAEGER_AGENT_PORT, like: localhost:6831
+	if e := os.Getenv(envAgentHost); e != "" {


nit: so if only the port is provided via env it won't be read here

For a moment, I thought this was in line with what the Java client was doing, but I'm clearly wrong.

yurishkuro · 2018-03-28T16:55:17Z

config/config_env.go

+// parseTags parses the given string into a collection of Tags.
+// Spec for this value:
+// - comma separated list of key=value
+// - value can be specified using the notation ${envVar:defaultValue}, where `envVar`


${envVar:defaultValue} - do we really need this? I.e. couldn't this be delegated to the shells where the env vars are defined?

I'm not sure. This was added by @objectiser, IIRC, so, he might have a case for it. If we decide to remove, I'd do it in a separate PR, as we should also remove it from the Java client.

yurishkuro · 2018-03-28T16:57:44Z

config/config_test.go

+		{
+			envVar:      envRPCMetrics,
+			value:       "NOT_A_BOOLEAN",
+			expectedErr: "strconv.ParseBool",


see my comment about wrapping errors - if it's implemented, the tests could be written without dependency on the internal wording of Go errors, we can simply be checking for our own strings like Cannot parse env var JAEGER_RPC_METRICS= NOT_A_BOOLEAN

jpkrohling · 2018-03-29T09:57:47Z

PR updated. I left the ${envVar:defaultValue} out of this, creating #279 to discuss it.

Adds support for configuring the tracer based on environment variables, similar to what is currently supported by the Java client. Closes jaegertracing#206 Signed-off-by: Juraci Paixão Kröhling <[email protected]>

yurishkuro · 2018-04-05T03:59:04Z

we should change JAEGER_SAMPLER_MANAGER_HOST_PORT to CONFIG_MGR_HOST_PORT, because that endpoint is used more than just for sampling configuration

robdefeo · 2018-04-15T19:28:21Z

what release version is the change scheduled for?

yurishkuro · 2018-04-15T19:57:32Z

released as 2.13

jpkrohling mentioned this pull request Mar 27, 2018

Support JAEGER_AGENT_HOST/PORT variables #207

Closed

pavolloffay reviewed Mar 27, 2018

View reviewed changes

jpkrohling force-pushed the 206-ConfigureClientViaEnvVars branch from 1e1299d to e85b0c6 Compare March 28, 2018 09:58

jpkrohling commented Mar 28, 2018

View reviewed changes

jpkrohling changed the title ~~WIP - Closes #206 - Added support for client configuration via env vars~~ Closes #206 - Added support for client configuration via env vars Mar 28, 2018

yurishkuro reviewed Mar 28, 2018

View reviewed changes

jpkrohling force-pushed the 206-ConfigureClientViaEnvVars branch from 0a861a7 to c844e00 Compare March 28, 2018 15:52

jpkrohling changed the title ~~Closes #206 - Added support for client configuration via env vars~~ Added support for client configuration via env vars Mar 28, 2018

jpkrohling force-pushed the 206-ConfigureClientViaEnvVars branch from c844e00 to 7fe69a0 Compare March 28, 2018 16:15

yurishkuro changed the title ~~Added support for client configuration via env vars~~ Add support for client configuration via env vars Mar 28, 2018

yurishkuro reviewed Mar 28, 2018

View reviewed changes

yurishkuro approved these changes Mar 28, 2018

View reviewed changes

jpkrohling force-pushed the 206-ConfigureClientViaEnvVars branch from 7fe69a0 to b255cfa Compare March 29, 2018 09:53

jpkrohling mentioned this pull request Mar 29, 2018

Remove support for default env var value #279

Closed

Added support for client configuration via env vars

2dd39a3

Adds support for configuring the tracer based on environment variables, similar to what is currently supported by the Java client. Closes jaegertracing#206 Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling force-pushed the 206-ConfigureClientViaEnvVars branch from b255cfa to 2dd39a3 Compare March 30, 2018 11:07

black-adder merged commit 6cc524e into jaegertracing:master Apr 3, 2018

jpkrohling mentioned this pull request Apr 5, 2018

Rename JAEGER_SAMPLER_MANAGER_HOST_PORT to JAEGER_SAMPLING_ENDPOINT #282

Closed

asp24 mentioned this pull request Apr 10, 2018

Add/enhance env variables support code-tool/jaeger-client-symfony-bridge#3

Open

eundoosong mentioned this pull request Oct 9, 2018

Support tags injection via JAEGER_TAGS environment variable #197

Closed

epkugelmass mentioned this pull request May 10, 2019

Jaeger Query expects agent at localhost jaegertracing/jaeger#747

Closed

		@@ -115,9 +135,23 @@ func (c Configuration) New(
		serviceName string,

Add support for client configuration via env vars #275

Add support for client configuration via env vars #275

Conversation

jpkrohling commented Mar 27, 2018 • edited Loading

codecov bot commented Mar 27, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay Mar 27, 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

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

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

jpkrohling commented Mar 28, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Mar 28, 2018

jpkrohling commented Mar 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling Mar 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Mar 29, 2018

yurishkuro commented Apr 5, 2018

robdefeo commented Apr 15, 2018

yurishkuro commented Apr 15, 2018

jpkrohling commented Mar 27, 2018 •

edited

Loading

codecov bot commented Mar 27, 2018 •

edited

Loading

pavolloffay Mar 27, 2018 •

edited

Loading

jpkrohling commented Mar 28, 2018 •

edited

Loading

jpkrohling Mar 30, 2018 •

edited

Loading