[Jaeger-Exporter] host default env var

[naseemkullah]

note: I was unable to add a test as this depends on an env var. running JAEGER_AGENT_HOST=foo npm run test showed me that it works though by failing the host checking tests hereby added.

Which problem is this PR solving?

• It helpes solve ENV_VAR for where to send spans opentelemetry-specification#172 but does not solve it entirely.

Short description of the changes

• Allowing the user to set env var JAEGER_AGENT_HOST in their app and the exporter will pick it up and use it as a value for host. setting host in the exporter config would trump this. Let me know if you would like the env var to take precedence.

[codecov-io]

Codecov Report

Merging #924 into master will increase coverage by 17.19%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #924       +/-   ##
===========================================
+ Coverage   77.73%   94.93%   +17.19%     
===========================================
  Files          74      248      +174     
  Lines        1752    11198     +9446     
  Branches      300     1070      +770     
===========================================
+ Hits         1362    10631     +9269     
- Misses        390      567      +177     

Impacted Files	Coverage Δ
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts	90.00% <100.00%> (ø)
.../opentelemetry-exporter-jaeger/test/jaeger.test.ts	100.00% <100.00%> (ø)
packages/opentelemetry-metrics/src/Metric.ts	96.66% <0.00%> (ø)
...y-plugin-http/test/functionals/http-enable.test.ts	96.28% <0.00%> (ø)
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts	100.00% <0.00%> (ø)
...ages/opentelemetry-core/src/platform/node/index.ts	100.00% <0.00%> (ø)
...s/opentelemetry-metrics/src/export/NoopExporter.ts	33.33% <0.00%> (ø)
...ntelemetry-tracing/test/MultiSpanProcessor.test.ts	97.43% <0.00%> (ø)
...ry-tracing/test/export/ConsoleSpanExporter.test.ts	100.00% <0.00%> (ø)
...y-tracing/test/export/InMemorySpanExporter.test.ts	100.00% <0.00%> (ø)
... and 179 more

[dyladan]

Please add a test that verifies the env var usage

[naseemkullah]

note: I was unable to add a test as this depends on an env var. running JAEGER_AGENT_HOST=foo npm run test showed me that it works though by failing the host checking tests hereby added.

@dyladan How can this be achieved?

[dyladan]

In the start of the test, set the variable process.env.JAEGER_AGENT_HOST = "host from env" then construct a jaeger exporter with no host property and test that it is working.

[naseemkullah]

In the start of the test, set the variable process.env.JAEGER_AGENT_HOST = "host from env" then construct a jaeger exporter with no host property and test that it is working.

I've never been able to achieve setting an env var for a single test. Is there any online examples of this?

[naseemkullah]

Also if both JAEGER_AGENT_HOST and options.host are set, which would make most sense to be authoritative?

[dyladan]

I've never been able to achieve setting an env var for a single test. Is there any online examples of this?

You just assign it like any other variable:

describe('suite', () => {
	// clean up for other tests
	afterEach(() => {
		delete process.env.JAEGER_AGENT_HOST;
	}));
	
	it('should respect jaeger host env variable', () => {
		process.env.JAEGER_AGENT_HOST = 'env-var';
		const exporter = new JaegerExporter({
			serviceName: 'test-service',
		});

		// assert host is env-var
	});

	it('should prioritize host option over env variable', () => {
		process.env.JAEGER_AGENT_HOST = 'env-var';
		const exporter = new JaegerExporter({
			serviceName: 'test-service',
			host: 'explicit-host'
		});

		// assert host is explicit-host
	});
});

[naseemkullah]

	afterEach(() => {
		delete process.env.JAEGER_AGENT_HOST;
	}));
	

I've never been able to achieve setting an env var for a single test. Is there any online examples of this?

You just assign it like any other variable:

describe('suite', () => {
	// clean up for other tests
	afterEach(() => {
		delete process.env.JAEGER_AGENT_HOST;
	}));
	
	it('should respect jaeger host env variable', () => {
		process.env.JAEGER_AGENT_HOST = 'env-var';
		const exporter = new JaegerExporter({
			serviceName: 'test-service',
		});

		// assert host is env-var
	});

	it('should prioritize host option over env variable', () => {
		process.env.JAEGER_AGENT_HOST = 'env-var';
		const exporter = new JaegerExporter({
			serviceName: 'test-service',
			host: 'explicit-host'
		});

		// assert host is explicit-host
	});
});

Amazing, not the first time I wanted this. I'm surprised I could not find any online docs for this.
Thanks!

[naseemkullah]

Wouldn't we want the env var to be authoritative? Am I wrong to say that it is generally easier to configure an env var on the fly rather than change the code?

[dyladan]

Wouldn't we want the env var to be authoritative? Am I wrong to say that it is generally easier to configure an env var on the fly rather than change the code?

Yes the ENV var is much easier to change, but if someone goes to the trouble of putting something in code you can be pretty sure they meant to do that. We could log if they are both set or something if you want to be extra careful.

[naseemkullah]

Wouldn't we want the env var to be authoritative? Am I wrong to say that it is generally easier to configure an env var on the fly rather than change the code?

Yes the ENV var is much easier to change, but if someone goes to the trouble of putting something in code you can be pretty sure they meant to do that. We could log if they are both set or something if you want to be extra careful.

Fair enough. Will skip logging at this time but will add a note about authority in README.