-
Notifications
You must be signed in to change notification settings - Fork 773
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 4 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; | ||
|
||
internal const string OTelAgentHostEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_HOST"; | ||
internal const string OTelAgentPortEnvVarKey = "OTEL_EXPORTER_JAEGER_AGENT_PORT"; | ||
|
||
public JaegerExporterOptions() | ||
{ | ||
string agentHostEnvVar = Environment.GetEnvironmentVariable(OTelAgentHostEnvVarKey); | ||
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,80 @@ | ||
// <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(JaegerExporterOptions.OTelAgentHostEnvVarKey, "jeager-host"); | ||
Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentPortEnvVarKey, "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(JaegerExporterOptions.OTelAgentPortEnvVarKey, "invalid"); | ||
|
||
var options = new JaegerExporterOptions(); | ||
|
||
Assert.Equal(6831, options.AgentPort); // use default | ||
} | ||
|
||
[Fact] | ||
public void JaegerExporterOptions_EnvironmentVariableNames() | ||
{ | ||
Assert.Equal("OTEL_EXPORTER_JAEGER_AGENT_HOST", JaegerExporterOptions.OTelAgentHostEnvVarKey); | ||
Assert.Equal("OTEL_EXPORTER_JAEGER_AGENT_PORT", JaegerExporterOptions.OTelAgentPortEnvVarKey); | ||
} | ||
|
||
private void ClearEnvVars() | ||
{ | ||
Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentHostEnvVarKey, null); | ||
Environment.SetEnvironmentVariable(JaegerExporterOptions.OTelAgentPortEnvVarKey, 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.
I would move this section to a new method - eg
SetOptionsViaEnvironmentVariables
.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.
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:
but it felt unnecessary complex
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.
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 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.
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.
Will do
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.
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. anOutOfMemoryException
.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.
this looks good.