Dev/twegner/issue 18 #111
Dev/twegner/issue 18 #111
Conversation
Merging open-telemetry/opentelemetry-dotnet master
…try-dotnet Merging upstream master
…gPropagationComponent
…es to be enforced.
Merging upstream master
src/OpenTelemetry/Trace/Tracer.cs
Outdated
| private readonly IBinaryFormat binaryFormat; | ||
| private readonly ITextFormat textFormat; | ||
|
|
||
| public Tracer(IRandomGenerator randomGenerator, IStartEndHandler startEndHandler, ITraceConfig traceConfig, IExportComponent exportComponent) | ||
| : this(randomGenerator, startEndHandler, traceConfig, exportComponent, null, null) |
There was a problem hiding this comment.
if it does not get export component, it should probably get exporter instead and use it in RecordSpanData?
There was a problem hiding this comment.
Great point. Have updated implementation to include exporter.
src/OpenTelemetry/Trace/Tracing.cs
Outdated
| /// Gets the tracer configuration. | ||
| /// </summary> | ||
| public static ITraceConfig TraceConfig => tracing.traceComponent.TraceConfig; | ||
| public static ITracer Tracer => TracerBase.NoopTracer; |
There was a problem hiding this comment.
is it intended? we have to create real tracer somewhere
There was a problem hiding this comment.
Another great point. Have updated implementation to create a tracer.
trwegner
requested review from
austinlparker,
discostu105 and
SergeyKanzhelev
as code owners
| //[Fact(Skip = "need to fix the way tracer being instantiated")] | ||
| //public void DefaultTraceExporter() | ||
| //{ | ||
| // Assert.Equal(ExportComponent.NewNoopExportComponent.GetType(), Tracing.ExportComponent.GetType()); |
There was a problem hiding this comment.
is it intentionally commented? should it be returned back or fixed?
It seems export component is still there
There was a problem hiding this comment.
Updated to restore tests
| @@ -51,7 +50,7 @@ public void ConfigureServices(IServiceCollection services) | |||
| services.AddSingleton<RequestsCollector>(); | |||
| services.AddSingleton<DependenciesCollectorOptions>(new DependenciesCollectorOptions()); | |||
| services.AddSingleton<DependenciesCollector>(); | |||
| services.AddSingleton<IExportComponent>(Tracing.ExportComponent); | |||
| services.AddSingleton<IExportComponent>(ExportComponent.NewNoopExportComponent); | |||
There was a problem hiding this comment.
i probably miss something - why do we need to register noop? It doesn't seem right
There was a problem hiding this comment.
Reverted to original
src/OpenTelemetry/Trace/Tracing.cs
Outdated
| // TODO(bdrutu): Add a config/argument for supportInProcessStores. | ||
| if (eventQueue is SimpleEventQueue) | ||
| { | ||
| this.ExportComponent = Export.ExportComponent.CreateWithoutInProcessStores(eventQueue); |
There was a problem hiding this comment.
so the only reason for export component are span stores?
But they are not even defined in the spec.
May be @SergeyKanzhelev could help here - why do we keep them in this repo?
There was a problem hiding this comment.
Are there additional changes I should make to address ExportComponent? It was not called out in the original issue. Should it be another issue instead?
There was a problem hiding this comment.
sure! I didn't realize it was not par of original issue, sorry about that
There was a problem hiding this comment.
No problem :) I will add a new issue for removing ExportComponent.
With last commits i'm moving from draft to actual PR. The changes are generally for removing the unnecessary component layer in the API referred to in issue #18. Removed components were primarily wrappers for a single class so largely redundant. I did encounter a number of challenges along the way since the changes introduced race conditions in several places which led to the use of concurrent-aware classes and/or locks. As a result some tests would fail randomly due to expected order in collections no longer being stable between runs. I was not able to find info in the spec indicated this was a problem so updated validation to allow varying order of results in the impacted tests.
Notes from draft PR:
@SergeyKanzhelev, @austinlparker, @lmolkova and @discostu105
Looking for feedback on these changes to ensure the work is addressing #18 appropriately. Currently still facing unit test failures in ViewManagerTest.cs and StatsRecorderTest.cs. Any observations in this area in particular would be appreciated.