Skip to content
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.

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

Add support for OTEL_EXPORTER_JAEGER_AGENT_HOST and OTEL_EXPORTER_JAEGER_AGENT_PORT #2123

Merged
merged 12 commits into from
Jul 15, 2021
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## Unreleased

* The `JaegerExporterOptions` defaults can be overridden using
`OTEL_EXPORTER_JAEGER_AGENT_HOST` and `OTEL_EXPORTER_JAEGER_AGENT_PORT`
envionmental variables as defined in the
[specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#jaeger-exporter).
([#2123](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2123))

## 1.1.0-rc1

Released 2021-Jun-25
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,11 @@ public void FailedExport(string exception)
{
this.WriteEvent(1, exception);
}

[Event(2, Message = "Failed to parse environment variable: '{0}', value: '{1}'.", Level = EventLevel.Warning)]
public void FailedToParseEnvironmentVariable(string name, string value)
{
this.WriteEvent(2, name, value);
}
}
}
27 changes: 27 additions & 0 deletions src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,41 @@
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using OpenTelemetry.Exporter.Jaeger.Implementation;

namespace OpenTelemetry.Exporter
{
public class JaegerExporterOptions
{
internal const int DefaultMaxPayloadSizeInBytes = 4096;

private const string OTelAgentHostEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_HOST";
private const string OTelAgentPortEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_PORT";
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@pellared pellared Jul 8, 2021

Choose a reason for hiding this comment

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

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:

Copy link
Member

Choose a reason for hiding this comment

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

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:

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);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed aad91eb


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

this looks good.

if (!string.IsNullOrEmpty(agentHostEnvVar))
{
this.AgentHost = agentHostEnvVar;
}

string agentPortEnvVar = Environment.GetEnvironmentVariable(OTelAgentPortEnvVarKey);
if (!string.IsNullOrEmpty(agentPortEnvVar))
{
if (int.TryParse(agentPortEnvVar, out var agentPortValue))
{
this.AgentPort = agentPortValue;
}
else
{
JaegerExporterEventSource.Log.FailedToParseEnvironmentVariable(OTelAgentPortEnvVarKey, agentPortEnvVar);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@pellared pellared Jul 9, 2021

Choose a reason for hiding this comment

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

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

?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@pellared pellared Jul 12, 2021

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to doing it as a separate PR.

}
}
}

/// <summary>
/// Gets or sets the Jaeger agent host. Default value: localhost.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// <copyright file="JaegerExporterOptionsTests.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using Xunit;

namespace OpenTelemetry.Exporter.Jaeger.Tests
{
public class JaegerExporterOptionsTests : IDisposable
{
public JaegerExporterOptionsTests()
{
this.ClearEnvVars();
}

public void Dispose()
{
this.ClearEnvVars();
}

[Fact]
public void JaegerExporterOptions_Defaults()
{
var options = new JaegerExporterOptions();

Assert.Equal("localhost", options.AgentHost);
Assert.Equal(6831, options.AgentPort);
Assert.Equal(4096, options.MaxPayloadSizeInBytes);
Assert.Equal(ExportProcessorType.Batch, options.ExportProcessorType);
}

[Fact]
public void JaegerExporterOptions_EnvironmentVariableOverride()
{
Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_HOST", "jeager-host");
Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_PORT", "123");

var options = new JaegerExporterOptions();

Assert.Equal("jeager-host", options.AgentHost);
Assert.Equal(123, options.AgentPort);
}

[Fact]
public void JaegerExporterOptions_InvalidPortEnvironmentVariableOverride()
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_PORT", "invalid");

var options = new JaegerExporterOptions();

Assert.Equal(6831, options.AgentPort); // use default
}

private void ClearEnvVars()
{
Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_HOST", null);
Environment.SetEnvironmentVariable("OTEL_EXPORTER_JAEGER_AGENT_PORT", null);
}
}
}