-
Notifications
You must be signed in to change notification settings - Fork 782
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
Changes from 3 commits
632208b
2158fb4
84f7766
aad91eb
5909df2
72e9c4e
c82e761
43e6bee
9c2c6ab
e4daac7
c608f14
9387fd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
||
public JaegerExporterOptions() | ||
{ | ||
string agentHostEnvVar = Environment.GetEnvironmentVariable(OTelAgentHostEnvVarKey); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would move this section to a new method - eg There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handled: 5909df2 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) 😃
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @reyang @cijothomas There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:There was a problem hiding this comment.
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:
opentelemetry-dotnet/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs
Lines 467 to 474 in a5b42fb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed aad91eb