-
Notifications
You must be signed in to change notification settings - Fork 765
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_BSP_EXPORT_* environmental variables #2219
Merged
cijothomas
merged 19 commits into
open-telemetry:main
from
pellared:support-OTEL_BSP-env-vars
Sep 3, 2021
Merged
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
6b2bb13
Add support for OTEL_BSP_EXPORT_* environmental variables
pellared 2ce1898
Handle errors
pellared 29e5f22
Update docs
pellared e2932be
Fix typo
pellared 84dab4e
Merge branch 'main' into support-OTEL_BSP-env-vars
cijothomas 36bffaa
Merge branch 'main' into support-OTEL_BSP-env-vars
cijothomas f679a64
Refactor BatchExportProcessorOptions
pellared b421b3d
Refactor
pellared 4625407
Merge branch 'main' into support-OTEL_BSP-env-vars
pellared 092ddea
Introduce BatchSpanExportProcessorOptions
pellared 8315017
Fix typo
pellared 188173d
Merge branch 'main' into support-OTEL_BSP-env-vars
pellared 7dfb935
Rename to BatchExportActivityProcessorOptions
pellared 917fd8d
Rename in usages
pellared 018b6a4
Update public API
pellared 864c113
Merge branch 'main' into support-OTEL_BSP-env-vars
pellared f737915
Merge branch 'main' into support-OTEL_BSP-env-vars
cijothomas c14094a
Merge branch 'main' into support-OTEL_BSP-env-vars
cijothomas ceae91e
Merge branch 'main' into support-OTEL_BSP-env-vars
cijothomas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
103 changes: 103 additions & 0 deletions
103
test/OpenTelemetry.Tests/Trace/BatchExportProcessorOptionsTest.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
// <copyright file="BatchExportProcessorOptionsTest.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; | ||
|
||
using Options = OpenTelemetry.BatchExportProcessorOptions<object>; | ||
|
||
namespace OpenTelemetry.Trace.Tests | ||
{ | ||
public class BatchExportProcessorOptionsTest : IDisposable | ||
{ | ||
public BatchExportProcessorOptionsTest() | ||
{ | ||
this.ClearEnvVars(); | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
this.ClearEnvVars(); | ||
} | ||
|
||
[Fact] | ||
public void BatchExportProcessorOptions_Defaults() | ||
{ | ||
var options = new Options(); | ||
|
||
Assert.Equal(30000, options.ExporterTimeoutMilliseconds); | ||
Assert.Equal(512, options.MaxExportBatchSize); | ||
Assert.Equal(2048, options.MaxQueueSize); | ||
Assert.Equal(5000, options.ScheduledDelayMilliseconds); | ||
} | ||
|
||
[Fact] | ||
public void BatchExportProcessorOptions_EnvironmentVariableOverride() | ||
{ | ||
Environment.SetEnvironmentVariable(Options.ExporterTimeoutEnvVarKey, "1"); | ||
Environment.SetEnvironmentVariable(Options.MaxExportBatchSizeEnvVarKey, "2"); | ||
Environment.SetEnvironmentVariable(Options.MaxQueueSizeEnvVarKey, "3"); | ||
Environment.SetEnvironmentVariable(Options.ScheduledDelayEnvVarKey, "4"); | ||
|
||
var options = new BatchExportProcessorOptions<object>(); | ||
|
||
Assert.Equal(1, options.ExporterTimeoutMilliseconds); | ||
Assert.Equal(2, options.MaxExportBatchSize); | ||
Assert.Equal(3, options.MaxQueueSize); | ||
Assert.Equal(4, options.ScheduledDelayMilliseconds); | ||
} | ||
|
||
[Fact] | ||
public void BatchExportProcessorOptions_InvalidPortEnvironmentVariableOverride() | ||
{ | ||
Environment.SetEnvironmentVariable(BatchExportProcessorOptions<object>.ExporterTimeoutEnvVarKey, "invalid"); | ||
|
||
var options = new Options(); | ||
|
||
Assert.Equal(30000, options.ExporterTimeoutMilliseconds); // use default | ||
} | ||
|
||
[Fact] | ||
public void BatchExportProcessorOptions_SetterOverridesEnvironmentVariable() | ||
{ | ||
Environment.SetEnvironmentVariable(BatchExportProcessorOptions<object>.ExporterTimeoutEnvVarKey, "123"); | ||
|
||
var options = new Options | ||
{ | ||
ExporterTimeoutMilliseconds = 89000, | ||
}; | ||
|
||
Assert.Equal(89000, options.ExporterTimeoutMilliseconds); | ||
} | ||
|
||
[Fact] | ||
public void BatchExportProcessorOptions_EnvironmentVariableNames() | ||
{ | ||
Assert.Equal("OTEL_BSP_EXPORT_TIMEOUT", Options.ExporterTimeoutEnvVarKey); | ||
Assert.Equal("OTEL_BSP_MAX_EXPORT_BATCH_SIZE", Options.MaxExportBatchSizeEnvVarKey); | ||
Assert.Equal("OTEL_BSP_MAX_QUEUE_SIZE", Options.MaxQueueSizeEnvVarKey); | ||
Assert.Equal("OTEL_BSP_SCHEDULE_DELAY", Options.ScheduledDelayEnvVarKey); | ||
} | ||
|
||
private void ClearEnvVars() | ||
{ | ||
Environment.SetEnvironmentVariable(Options.ExporterTimeoutEnvVarKey, null); | ||
Environment.SetEnvironmentVariable(Options.MaxExportBatchSizeEnvVarKey, null); | ||
Environment.SetEnvironmentVariable(Options.MaxQueueSizeEnvVarKey, null); | ||
Environment.SetEnvironmentVariable(Options.ScheduledDelayEnvVarKey, null); | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
These are specific to batch span processors, we need some logic to make sure it is not affecting other scenario (e.g. metrics).
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 you have any proposal on how to approach this?
I think that the root problem is that the .NET SDK tries to use common (premature?) abstractions to implement multiple OTel specification components. E.g. we have a common exporter base class for both metrics and traces exporter. I am aware it reduces some code duplication but it tightly couples the API and it may be hard to maintain backward compatibility going this way.
Can we use
BatchExportProcessorOptions
for tracing (to not break SemVer) and create aBatchMetricsExportProcessorOptions
for metrics SDK?Moreover, I think we should consider separating the metrics and traces SDKs.
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.
What if we did something like...
I'm not really a fan of duck typing in generics like this but trying to introduce classes into the hierarchy now will probably be a breaking change.
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.
@CodeBlanch thanks 👍
That is why I suggest changing the metrics SDK as AFAIK it od not stable yet. And leave the tracing SDK as it is.
Your proposal would work and probably it is the most pragmatic idea for this PR. But maybe it is worth taking a step back and make more refactoring in separate PR(s).
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.
@CodeBlanch
I noticed that
OtlpMetricsExporter
usesOtlpExporterOptions
which has aBatchExportProcessorOptions<Activity>
.Personally, I would prefer to redesign the
OtlpMetricsExporter
. The main repository's readme says that metrics are experimental so probably it is acceptable. Then we could redesignOtlpMetricsExporter
to not haveBatchExportProcessorOptions
as it is not used anyway. I think we could refactorOtlpMetricsExporter
as a separate issue+PR.Regarding this PR I see the following options:
We could just improve the
BatchExportProcessorOptions
docs and say that this option is designed for the tracing SDK.Instead of "duck typing in generics" we could also possibly create a subclass
BatchSpanExportProcessorOptions
and use it like this:not sure how much of a breaking change - I guess 99% of the existing code should simply work. Personally, I find such "common property grouping" as a code smell and possible premature abstraction. Will the exporters handle all of the possible options?
Use the "duck typing in generics" proposed here
Shouldn't the tracing and metrics packages be distributed separately to avoid confusion? How the user would distinguish between stable and unstable API?
@reyang PTAL as well
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.
After the SIG meeting, I have an impression that this would be the preferred way forward:
OtlpMetricsExporter
should not referenceBatchExportProcessorOptions<Activity>
. Done here:OtlpMetricsExporter
should not referenceBatchExportProcessorOptions<Activity>
#2246BatchSpanExportProcessorOptions
as a subclass ofBatchExportProcessorOptions<Activity>
. Addressed here: 092ddea