Support Environment Variables for JaegerSpanExporter configuration #1114

rydzykje · 2020-09-15T13:02:09Z

Description

I've added possibility to configure JaegerSpanExporter by Environment Variables defined by OTEL_SPEC

Fixes # (#1056)

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

I've used the jaeger_explorter_example.py script to check if OTEL_ENVS are properly used.
Updated test_jaeger_exporter.py

Checklist:

Followed the style guidelines of this project
Change logs have been updated
Unit tests have been added
Documentation has been updated

linux-foundation-easycla · 2020-09-15T14:27:20Z

The committers are authorized under a signed CLA.

✅ Mateusz 'mat' Rumian (7f88918, a215f13, aac2612)

codeboten

The change is looking pretty good!

Please add a changelog entry to note the change in method signature and the new support for env variables. It would be good to have those variables added to the docs as well, or at least a link to the otel spec.

One more question in the code regarding precedence

codeboten · 2020-09-16T00:11:54Z

exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py

+            os.environ.get(OTEL_ENVS["collector_endpoint"])
+            or collector_endpoint
+        )
+        self.username = os.environ.get(OTEL_ENVS["username"]) or username


I would expect the or to be reversed here username or os.environ.get(OTEL_ENVS["username"]) as to ensure that if someone overrides the variable in the code, it takes precedence over environment variables. It's the model I followed for the zipkin environment variables https://github.com/open-telemetry/opentelemetry-python/pull/1064/files#diff-eb584b1fd47b0e9ede2d87c686b87a1aR113. We should be consistent one way or another.

I've added requested changes :)

lzchen · 2020-09-16T16:18:57Z

exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py

-        collector_port=None,
-        collector_endpoint=DEFAULT_COLLECTOR_ENDPOINT,
-        collector_protocol=DEFAULT_COLLECTOR_PROTOCOL,
+        collector_endpoint=None,


Isn't the default supposed to be http://localhost:14250?

I think in this case it shouldn't. Previously collector_endpoint was related to /api/traces?format=jaeger.thrift and it was just a part of complete endpoint address. If we set collector_endpoint default then it will be used over agent which was default implementation.

Any reason why the specs would have that is the default value then?

If we set collector_endpoint default then it will be used over agent which was default implementation

I'm also not sure what you mean by the above statement.

I just want to say that in previous implementation collector by default was not used and collector_endpoint was just an end part of complete URL. In current implementation there's no need to specify host,port and endpoint separately because they will be provided by one environment variable called OTEL_EXPORTER_JAEGER_ENDPOINT coming from otel specification.

Correct, I am asking whether the collector_endpoint value should default to http://localhost:14250 as defined by the spec if the value is not explicitly passed into the exporter AND the environment variable is not configured.

According to our discussion on the Gitter. We've decided to do not set the collector_endpoint default because it is causing not entirely clear path of the used protocol to export spans. Separate issue and PR should be created for this purpose.

Hmm, looking at java's implementation, it looks like they only have the concept of the collector endpoint. This leads me to believe that we should still have the default value for collector_endpoint, and if there's anything that errors with the creation of the Collector, then we would use the AgentClientUDP instead. So we should still set the default value to http://localhost:14250.

@codeboten @rydzykje
Thoughts on this?

Looking at the code, I don't know if there's any scenario where the .constructor property would return None if the default of collector_endpoint is always set. Based on the jaeger docs, it would appear that only the agent configuration is usually set by default, and a collector endpoint would have to be explicitly set:

If we want Jaeger client libraries to send trace data directly to collectors, we must provide them with a URL of the HTTP endpoint. It means that our applications require additional configuration containing this parameter, especially if we are running multiple Jaeger installations

I wonder if the spec should default to not having a collector endpoint. You're right @lzchen that other implementations only have the concept of an endpoint, go does the same thing.

I would be ok to go ahead with this implementation and raise an issue with the spec about this.

exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py

rydzykje · 2020-09-21T22:45:48Z

@codeboten @lzchen there's one more thing which stays configured without environment variable - service_name. In current implementation this is the only one thing which has to be provided directly in the code during JaegerSpanExporter instantiation. Provided specification doesn't clarify the env for this purpose.

codeboten · 2020-09-22T15:24:05Z

exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py

+            Configuration().EXPORTER_JAEGER_ENDPOINT or collector_endpoint
+        )
+        self.username = username or Configuration().EXPORTER_JAEGER_USER
+        self.password = password or Configuration().EXPORTER_JAEGER_PASSWORD


It looks like for username/password, precedence is given to the configured parameter, whereas for agent host,port and endpoint, the precedence is given to the environment variable. this would be confusing to me as a user.

A good way to test this would be to ensure the precedence is consistent when both environment variables and the constructor is called with all parameters.

codeboten · 2020-10-01T15:45:49Z

@rydzykje any updates on this PR?

rydzykje · 2020-10-01T19:48:37Z

@rydzykje any updates on this PR?

I'm sorry I had a lot of things to do in my job :) I will take care of it tomorrow.

codeboten · 2020-10-01T21:31:04Z

I'm sorry I had a lot of things to do in my job :) I will take care of it tomorrow.

No worries! Thanks for the quick response, will await your changes.

lzchen · 2020-10-08T19:47:55Z

@rydzykje
Hey if you could maybe add a comment and tag us directly about which changes/comments that you've addressed every time you make a commit that you want us to review that would be very helpful.

codeboten

Thanks for the changes @rydzykje, LGTM! Just one non-blocking comment

codeboten · 2020-10-09T15:09:29Z

exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py

@@ -177,6 +177,22 @@ def shutdown(self):
        pass


+def _parameter_setter(param, env_variable, default):


I wonder if this should be in the configuration object to ensure the same order of precedence is used everywhere.

rydzykje requested a review from a team September 15, 2020 13:02

rydzykje changed the title ~~Support Environment Variables for JaegerSpanExporter~~ Support Environment Variables for JaegerSpanExporter configuration Sep 15, 2020

codeboten suggested changes Sep 16, 2020

View reviewed changes

lzchen reviewed Sep 16, 2020

View reviewed changes

codeboten added the release:required-for-ga To be resolved before GA release label Sep 17, 2020

rydzykje added 6 commits September 18, 2020 20:14

Introduce OTEL Environment Variables support

d180fd6

Update and add OTEL Envs test

f7847dd

Apply lint comments

0913ee0

Reverse setup of username and password

79cee16

Update CHANGELOG.md

2ff3c9f

Add configuration information

3d8f333

rydzykje force-pushed the rydzykje-support_env_var_conf_jaeger branch from 1bc93ce to 3d8f333 Compare September 18, 2020 18:19

lzchen reviewed Sep 21, 2020

View reviewed changes

exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py Outdated Show resolved Hide resolved

rydzykje added 4 commits September 21, 2020 20:34

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

a6bceb1

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

f98c6db

Introduce Configuration

d466113

Update tests

50a9312

codeboten reviewed Sep 22, 2020

View reviewed changes

rydzykje and others added 4 commits October 2, 2020 19:38

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

ff5ae8d

Swap username and password with env vars

952549c

Introduce parameter setter, constructor parameters over env vars

b3c0da6

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

8cfc4ed

codeboten assigned codeboten and lzchen Oct 8, 2020

codeboten self-requested a review October 8, 2020 16:35

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

36e9819

codeboten approved these changes Oct 9, 2020

View reviewed changes

codeboten requested a review from lzchen October 9, 2020 16:45

lzchen approved these changes Oct 14, 2020

View reviewed changes

lzchen and others added 3 commits October 14, 2020 11:20

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

d536a91

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

406486e

Merge branch 'master' into rydzykje-support_env_var_conf_jaeger

47ab344

codeboten merged commit f669b67 into open-telemetry:master Oct 15, 2020

codeboten mentioned this pull request Oct 19, 2020

Support env variable configuration for Jaeger exporter #1056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Environment Variables for JaegerSpanExporter configuration #1114

Support Environment Variables for JaegerSpanExporter configuration #1114

rydzykje commented Sep 15, 2020 •

edited

Loading

linux-foundation-easycla bot commented Sep 15, 2020 •

edited

Loading

codeboten left a comment •

edited

Loading

codeboten Sep 16, 2020

rydzykje Sep 18, 2020

lzchen Sep 16, 2020

mat-rumian Sep 16, 2020

lzchen Sep 17, 2020

rydzykje Sep 18, 2020

lzchen Sep 20, 2020

rydzykje Sep 21, 2020

lzchen Oct 5, 2020 •

edited

Loading

lzchen Oct 12, 2020

codeboten Oct 14, 2020

codeboten Oct 14, 2020

rydzykje commented Sep 21, 2020

codeboten Sep 22, 2020

codeboten commented Oct 1, 2020

rydzykje commented Oct 1, 2020

codeboten commented Oct 1, 2020

lzchen commented Oct 8, 2020

codeboten left a comment

codeboten Oct 9, 2020

		@@ -177,6 +177,22 @@ def shutdown(self):
		pass


		def _parameter_setter(param, env_variable, default):

Support Environment Variables for JaegerSpanExporter configuration #1114

Support Environment Variables for JaegerSpanExporter configuration #1114

Conversation

rydzykje commented Sep 15, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Sep 15, 2020 • edited Loading

codeboten left a comment • 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

lzchen Oct 5, 2020 • 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

rydzykje commented Sep 21, 2020

Choose a reason for hiding this comment

codeboten commented Oct 1, 2020

rydzykje commented Oct 1, 2020

codeboten commented Oct 1, 2020

lzchen commented Oct 8, 2020

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rydzykje commented Sep 15, 2020 •

edited

Loading

linux-foundation-easycla bot commented Sep 15, 2020 •

edited

Loading

codeboten left a comment •

edited

Loading

lzchen Oct 5, 2020 •

edited

Loading