Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT #2123

pellared · 2021-07-08T07:55:35Z

Partially addresses #1453.

This feature is important so that we can reuse the SDK in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation

Changes

The JaegerExporterOptions defaults can be overriden using OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT envionmental variables as defined in the specification.

The implementation is similar to OTel Go, OTel Python, and other languages as well.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Design discussion issue SDK configuration from ENV Variable #1453
Changes in public API reviewed

TODO (next PRs)

…GER_AGENT_PORT

codecov · 2021-07-08T08:12:59Z

Codecov Report

Merging #2123 (9387fd8) into main (56357fc) will decrease coverage by 0.04%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #2123      +/-   ##
==========================================
- Coverage   85.04%   84.99%   -0.05%     
==========================================
  Files         187      187              
  Lines        6103     6124      +21     
==========================================
+ Hits         5190     5205      +15     
- Misses        913      919       +6

Impacted Files	Coverage Δ
...Jaeger/Implementation/JaegerExporterEventSource.cs	`53.84% <57.14%> (+3.84%)`	⬆️
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs	`84.21% <78.57%> (-15.79%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`79.41% <0.00%> (-2.95%)`	⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs	`62.50% <0.00%> (+6.25%)`	⬆️

RassK · 2021-07-08T10:10:02Z

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs

+
+        public JaegerExporterOptions()
+        {
+            string agentHostEnvVar = Environment.GetEnvironmentVariable(OTelAgentHostEnvVarKey);


I would move this section to a new method - eg SetOptionsViaEnvironmentVariables.

Waiting for feedback (e.g. upvotes) from others as this may be subjective. This would be the only call in this constructor so it might be seen as an unnecessary level of indirection.

I have no strong opinion here.

Initially, I thought of doing something like:

this.AgentHost = GetStringFromEnvironmentVariable(OTelAgentHostEnvVarKey, "localhost"); this.AgentPort = GetIntFromEnvironmentVariable(OTelAgentPortEnvVarKey, 6831, value => JaegerExporterEventSource.Log.FailedToParseEnvironmentVariable(OTelAgentPortEnvVarKey, value));

but it felt unnecessary complex

I kind of like these "general" methods but deferring until more usage seems reasonable.

Please wrap this inside try..catch incase user doesn't have permission to read, it'll throw.

Handled: 5909df2
@cijothomas Do we want to have another event for a generic Exception? Personally, I do not like it b/c we can catch e.g. an OutOfMemoryException.

this looks good.

RassK · 2021-07-08T10:13:21Z

Should we add option to 'DisableEnvironmentVariablesRead' ? Currently it's a new feature that is forcefully on - so to avoid conflicts when there are multiple applications on the same server instance, there is a way to turn it off.

E: Code has anyway higher importance... but then every single env variable has to be manually neutralized. Which still would cause unwanted feature to be turned on.

pellared · 2021-07-08T13:39:41Z

Should we add option to 'DisableEnvironmentVariablesRead' ? Currently it's a new feature that is forcefully on - so to avoid conflicts when there are multiple applications on the same server instance, there is a way to turn it off.

From my experience (especially for cloud native apps) the env vars are set per process. IT is even possible it off in code by simply:

Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_HOST", null);
Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_PORT", null);

pellared · 2021-07-08T13:41:10Z

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs

+        private const string OTelAgentHostEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_HOST";
+        private const string OTelAgentPortEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_PORT";


do we want to have such keys as public static readonly?

My vote is to keep as constants. But I would make them internal const string so you could use them in your unit tests.

For me using the hardcoded strings makes the test more readable. Especially that the env vars are defined by specs and won't likely change. Moreover it is a safeguard for against changing the const by mistake.

Therefore maybe let's keep it as it is? I tried to keep the code similar to OTEL_RESOURCE_ATTRIBUTES implementation:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/6b7f2dd77cf9d37260a853fcc95f7b77e296065d/test/OpenTelemetry.Tests/Resources/ResourceTest.cs

https://github.com/open-telemetry/opentelemetry-dotnet/blob/6b7f2dd77cf9d37260a853fcc95f7b77e296065d/test/OpenTelemetry.Tests/Resources/OtelEnvResourceDetectorTest.cs

https://github.com/open-telemetry/opentelemetry-dotnet/blob/6b7f2dd77cf9d37260a853fcc95f7b77e296065d/src/OpenTelemetry/Resources/OtelEnvResourceDetector.cs

IMO it is an SSOT (single source of truth) principle type of thing. If it does need to ever change, it should be in one spot. Those other spots are misbehaving 🤣 Here's a good one:

opentelemetry-dotnet/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs

Lines 467 to 474 in a5b42fb

if (expectedStatusCode == StatusCode.Error)

{

Assert.Contains(jaegerSpan.Tags, t => t.Key == JaegerActivityExtensions.JaegerErrorFlagTagName && t.VType == JaegerTagType.BOOL && (t.VBool ?? false));

}

else

{

Assert.DoesNotContain(jaegerSpan.Tags, t => t.Key == JaegerActivityExtensions.JaegerErrorFlagTagName);

}

Addressed aad91eb

CodeBlanch · 2021-07-08T16:42:35Z

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs

+                }
+                else
+                {
+                    JaegerExporterEventSource.Log.FailedToParseEnvironmentVariable(OTelAgentPortEnvVarKey, agentPortEnvVar);


@cijothomas Should we actually throw here? I think typically we do for misconfiguration during startup.

My answer would be no.

It is currently unspecified according to the spec.

Looking at the other part of the spec, it seems the general spirit is that the SDK should handle environment variable in a graceful way (e.g. if some environment variable wouldn't make sense, fall back to the default value rather than fail the app).

I think the reason is that environment variable might be invasive (e.g. a child process might inherit environment variable, someone might set global environment variable unintentionally), and we don't want that to crash the app.

Just wondering in the case of a typo how visible will be that? Are the host and port logged in case of error sending to a destination? Perhaps log the host, port (all the options in general) at startup so one can easily check the effective configuration?

@cijothomas What do you think of logging the options as an event with EventLevel.Informational (or even EventLevel.Verbose) e.g. here:

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs

Line 52 in a5b42fb

?

As long as we don't log credential by default, I'm supportive (Information/Verbose both sound good to me) 😃

If the environment variable is something like OTEL_EXPORTER_JAEGER_AGENT_PASSWORD, we should not log it by default (or we should replace the password with something like ******).

If the user put credential in OTEL_EXPORTER_JAEGER_AGENT_PORT, it's their problem, we will log it (rather than trying to do some smart detection and guess if it could be credential).

@reyang @cijothomas
Should I do it as a separate PR?
Personally, I see it as a separate feature and maybe it would be worth adding a dedicated point in the changelog for it.
I added a "TODO (next PRs)" section in the PR description.

+1 to doing it as a separate PR.

pjanotti

LGTM

pjanotti

@pellared could you please also update https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Exporter.Jaeger#readme to include the env vars.

test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs

cijothomas · 2021-07-09T18:25:07Z

@pellared @pjanotti Is there a timeline you are looking to get this change as part of a stable release?
We are planning to release 1.1.0 stable today/monday, and the next planned stable release is in Nov 2021. So by default, this change will go in the 1.2.0 releasing Nov 2021.
https://github.com/open-telemetry/opentelemetry-dotnet/milestones

pjanotti · 2021-07-09T18:30:48Z

@cijothomas thanks for considering the release timelines. Auto-Instrumentation wouldn't be in a stable release soon so we will have to live with provisional releases between 1.1.0 and 1.2.0 while we migrate auto-instrumentation to the SDK.

pellared · 2021-07-12T08:43:45Z

@pellared could you please also update https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Exporter.Jaeger#readme to include the env vars.

@pjanotti Addressed: c82e761

PS.
@cijothomas @reyang Maybe it would be good to have a table of supported environmental variables in the "root" README? Especially that I have not seen any docs for OTEL_RESOURCE_ATTRIBUTES in this repository.

src/OpenTelemetry.Exporter.Jaeger/README.md

cijothomas · 2021-07-12T16:37:05Z

@pellared could you please also update https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Exporter.Jaeger#readme to include the env vars.

@pjanotti Addressed: c82e761

PS.
@cijothomas @reyang Maybe it would be good to have a table of supported environmental variables in the "root" README? Especially that I have not seen any docs for OTEL_RESOURCE_ATTRIBUTES in this repository.

I think the root readme should be really small!
The resource env var support - can be documented in the SDK's readme page, as thats the project dealing with Resource feature/populating from env.

cijothomas · 2021-07-12T16:40:33Z

To avoid adding last minute changes to the 1.1.0 release, this PR will be merged after 1.1.0 is shipped (later today).

cijothomas

Blocking, just to prevent accidental merge.

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

cijothomas · 2021-07-14T18:56:56Z

Ready to merge as 1.1.0 ship has sailed. Will wait till today evening to see if there are any feedbacks, and then merge.

CodeBlanch

LGTM

Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAE…

632208b

…GER_AGENT_PORT

pellared requested a review from a team July 8, 2021 07:55

pellared added 2 commits July 8, 2021 09:57

Fix typo

2158fb4

Lint

84f7766

RassK reviewed Jul 8, 2021

View reviewed changes

pellared commented Jul 8, 2021

View reviewed changes

CodeBlanch reviewed Jul 8, 2021

View reviewed changes

pellared requested review from reyang and CodeBlanch July 8, 2021 17:27

Refactor constant usage

aad91eb

pjanotti approved these changes Jul 9, 2021

View reviewed changes

pjanotti reviewed Jul 9, 2021

View reviewed changes

cijothomas reviewed Jul 9, 2021

View reviewed changes

test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterOptionsTests.cs Show resolved Hide resolved

CodeBlanch mentioned this pull request Jul 9, 2021

Enable zipkin endpoint configuration via environment variable #1924

Merged

3 tasks

pellared added 3 commits July 12, 2021 09:31

Handle a case when permission to read env var is missing

5909df2

Add JaegerExporterOptions_SetterOverridesEnvironmentVariable

72e9c4e

Update docs

c82e761

Fix SA1413

43e6bee

pellared requested review from cijothomas, RassK and pjanotti July 12, 2021 08:59

cijothomas reviewed Jul 12, 2021

View reviewed changes

src/OpenTelemetry.Exporter.Jaeger/README.md Outdated Show resolved Hide resolved

cijothomas approved these changes Jul 12, 2021

View reviewed changes

Merge branch 'main' into jeager-env-vars

9c2c6ab

cijothomas added the pr:do-not-merge label Jul 12, 2021

cijothomas requested changes Jul 12, 2021

View reviewed changes

pellared and others added 2 commits July 12, 2021 20:32

Update src/OpenTelemetry.Exporter.Jaeger/README.md

e4daac7

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

Merge branch 'main' into jeager-env-vars

c608f14

pellared requested a review from cijothomas July 14, 2021 15:55

cijothomas approved these changes Jul 14, 2021

View reviewed changes

pjanotti approved these changes Jul 14, 2021

View reviewed changes

CodeBlanch approved these changes Jul 15, 2021

View reviewed changes

Merge branch 'main' into jeager-env-vars

9387fd8

cijothomas merged commit a9fed42 into open-telemetry:main Jul 15, 2021

pellared deleted the jeager-env-vars branch July 15, 2021 05:12

cijothomas removed the pr:do-not-merge label Jul 21, 2021

CodeBlanch mentioned this pull request Jul 30, 2021

Add support for OTEL_SERVICE_NAME environmental variable #2209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT #2123

Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT #2123

pellared commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

RassK Jul 8, 2021

pellared Jul 8, 2021

pjanotti Jul 9, 2021

cijothomas Jul 9, 2021

pellared Jul 9, 2021

pellared Jul 12, 2021

cijothomas Jul 12, 2021

RassK commented Jul 8, 2021 •

edited

Loading

pellared commented Jul 8, 2021

pellared Jul 8, 2021

CodeBlanch Jul 8, 2021

pellared Jul 8, 2021 •

edited

Loading

CodeBlanch Jul 8, 2021

pellared Jul 9, 2021

CodeBlanch Jul 8, 2021

reyang Jul 8, 2021

pjanotti Jul 9, 2021

pellared Jul 9, 2021 •

edited

Loading

reyang Jul 9, 2021

pellared Jul 12, 2021 •

edited

Loading

cijothomas Jul 12, 2021

pjanotti left a comment

pjanotti left a comment

cijothomas commented Jul 9, 2021

pjanotti commented Jul 9, 2021

pellared commented Jul 12, 2021 •

edited

Loading

cijothomas commented Jul 12, 2021

cijothomas commented Jul 12, 2021

cijothomas left a comment

cijothomas commented Jul 14, 2021

CodeBlanch left a comment

		private const string OTelAgentHostEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_HOST";
		private const string OTelAgentPortEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_PORT";

	if (expectedStatusCode == StatusCode.Error)
	{
	Assert.Contains(jaegerSpan.Tags, t => t.Key == JaegerActivityExtensions.JaegerErrorFlagTagName && t.VType == JaegerTagType.BOOL && (t.VBool ?? false));
	}
	else
	{
	Assert.DoesNotContain(jaegerSpan.Tags, t => t.Key == JaegerActivityExtensions.JaegerErrorFlagTagName);
	}

Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT #2123

Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT #2123

Conversation

pellared commented Jul 8, 2021 • edited Loading

Changes

TODO (next PRs)

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

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

RassK commented Jul 8, 2021 • edited Loading

pellared commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Jul 8, 2021 • 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

pellared Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

cijothomas commented Jul 9, 2021

pjanotti commented Jul 9, 2021

pellared commented Jul 12, 2021 • edited Loading

cijothomas commented Jul 12, 2021

cijothomas commented Jul 12, 2021

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas commented Jul 14, 2021

CodeBlanch left a comment

Choose a reason for hiding this comment

pellared commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

RassK commented Jul 8, 2021 •

edited

Loading

pellared Jul 8, 2021 •

edited

Loading

pellared Jul 9, 2021 •

edited

Loading

pellared Jul 12, 2021 •

edited

Loading

pellared commented Jul 12, 2021 •

edited

Loading