Add support for multiple pipelines in OpenTelemetryBuilder with Activity by cijothomas · Pull Request #735 · open-telemetry/opentelemetry-dotnet

cijothomas · 2020-06-17T06:57:47Z

Towards 1c in #684

Changes

Add ability to have multiple processing pipeline, exposed via AddProcessorPipeline on OpenTelemetryBuilder. (this capability already existed in Span based pipeline)

Checklist

I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public API reviewed

The PR will automatically request reviews from code owners and trigger CI build and tests.

reyang · 2020-06-17T15:21:45Z

src/OpenTelemetry/Trace/Export/Internal/BroadcastActivityProcessor.cs

+
+namespace OpenTelemetry.Trace.Export.Internal
+{
+    internal class BroadcastActivityProcessor : ActivityProcessor, IDisposable


The name is misleading since "broadcast" normally means there is no need to specify the target.

Probably borrow from Java/Python:

Java MultiSpanExporter

Python SynchronousMultiSpanProcessor

FanOutActivityProcessor? 🤷

reyang · 2020-06-17T15:27:04Z

src/OpenTelemetry/Trace/Export/Internal/BroadcastActivityProcessor.cs

+{
+    internal class BroadcastActivityProcessor : ActivityProcessor, IDisposable
+    {
+        private readonly IEnumerable<ActivityProcessor> processors;


Potential perf concern:
IEnumerable<T> implies a new enumerator object creation on the hot path (each span will result in a foreach (var processor in this.processors).
Probably worth considering array or list (which maintains the count and can be accessed via index).

reyang · 2020-06-17T15:27:55Z

src/OpenTelemetry/Trace/Export/Internal/BroadcastActivityProcessor.cs

+                throw new ArgumentException($"{nameof(processors)} collection is empty");
+            }
+
+            this.processors = processors;


Should this be a shallow copy? What if the passed in processors got updated later?

CodeBlanch · 2020-06-17T15:28:30Z

test/OpenTelemetry.Tests/Implementation/Trace/Config/ActivityProcessorPipelineTests.cs

+using OpenTelemetry.Trace.Export;
+using Xunit;
+
+namespace OpenTelemetry.Tests.Impl.Trace.Config


nit: "Impl" -> "Implementation"

Yes. we need to rearrange this quite a lot. "impl" is used in many other places as well. - i'll do it as a separate PR where we will do consistent namespace. If product is namespace, then test would be namespace.test. something like that. (currently its a mix with no consistency)

CodeBlanch · 2020-06-17T15:29:03Z

src/OpenTelemetry/Trace/Export/Internal/BroadcastActivityProcessor.cs

+            }
+        }
+
+        public override void OnStart(Activity activity)


nit: Probably more common to have Start before End.

reyang · 2020-06-17T15:30:03Z

src/OpenTelemetry/Trace/Export/Internal/BroadcastActivityProcessor.cs

+            return Task.WhenAll(tasks);
+        }
+
+        public void Dispose()


This is different from what I've normally seen, worth double checking https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#implement-the-dispose-pattern.

IMO because it's internal it's fine as-is. That being said, OpenTelemetrySdk isn't calling shutdown or dispose on the ActivityProcessor it is maintaining. That's a problem. We need to flush out spans & perform cleanup on app shutdown right? I think OpenTelemetrySdk.Dispose should call ActivityProcessor.Dispose which should invoke ActivityProcessor.ShutdownAsync? We might want to look at using IAsyncDisposable for that.

There are few things we need to address:

Improvements to SimpleSpanProcessor. #674 SimpleSpanProcessor needs to be a synchronous call (e.g. when the control is returned to the caller, the span got processed - current it is not guaranteed).

The life-cycle management of processors/exporters, related to this comment. For example, if we have the same processor/exporter registered twice, would we flush the exporter on the first OnEnd, or maintain a refcount and call OnEnd during Dispose? Does OnEnd need to be ref-counted as well?

We need to have a deterministic flush semantic. By default I would imagine we want to flush traces/logs before app shutdown, metrics is something debatable. And we need to give option to the user whether they want to wait indefinitely or have a timeout or no wait at all.

Agree with all of the above - But my goal with this PR is to just mimic the existing behavior with Spans (this pr was made with pure 'copy paste & minor adjustments' ). I'd prefer to get the parity 1st, then fix underlying issues.

Let me know if this approach is okay?

Yeah, these are not blockers for this PR.

IMO because it's internal it's fine as-is. That being said, OpenTelemetrySdk isn't calling shutdown or dispose on the ActivityProcessor it is maintaining. That's a problem. We need to flush out spans & perform cleanup on app shutdown right? I think OpenTelemetrySdk.Dispose should call ActivityProcessor.Dispose which should invoke ActivityProcessor.ShutdownAsync? We might want to look at using IAsyncDisposable for that.

Yes this is needed. Modified OTSDK to call dispose on the Activityprocessor it has.

Will follow up on why Span was not calling Shutdown in SimpleProcessor! (Was doing it right for batching one). Anyway - both will be fixed in next PR.

pjanotti

LGTM

pjanotti · 2020-06-17T23:30:52Z

src/OpenTelemetry/Trace/Export/Internal/BroadcastActivityProcessor.cs

+                tasks.Add(processor.ShutdownAsync(cancellationToken));
+            }
+
+            return Task.WhenAll(tasks);


The cancellationToken is passed to all ShutdownAsync but if one misbehaves this line can wait forever. Is this relying on cancellation by the caller?

The current code is vulnerable to one misbehaving processor. Not just in shutdown but even in start/stop as well. We need to address it properly.

…b.com/cijothomas/opentelemetry-dotnet into cijothomas/activityprocessingpipeline

cijothomas · 2020-06-18T00:41:14Z

Merging this as this makes Activity processing pipeline as same features as Span pipeline.
(Fully agree about a lot of issues with both :), will address them as follow ups)

Add support for multiple pipelines in OpenTelemetryBuilder with Activity

98800c8

cijothomas requested review from CodeBlanch, MikeGoldsmith, SergeyKanzhelev, bruno-garcia, discostu105, lmolkova and pjanotti as code owners June 17, 2020 06:57

cijothomas added 2 commits June 17, 2020 00:16

change Setpipeline toAddpipeline

481ff91

Merge branch 'master' into cijothomas/activityprocessingpipeline

08e6967

cijothomas requested a review from a team June 17, 2020 07:40

reyang reviewed Jun 17, 2020

View reviewed changes

CodeBlanch reviewed Jun 17, 2020

View reviewed changes

reyang reviewed Jun 17, 2020

View reviewed changes

cijothomas mentioned this pull request Jun 17, 2020

Epic - Span API -> Activity API #684

Closed

32 tasks

Merge branch 'master' into cijothomas/activityprocessingpipeline

4b14f66

pjanotti approved these changes Jun 17, 2020

View reviewed changes

cijothomas added 2 commits June 17, 2020 17:17

Dispose activityprocessor

be999b9

Merge branch 'cijothomas/activityprocessingpipeline' of https://githu…

8868edc

…b.com/cijothomas/opentelemetry-dotnet into cijothomas/activityprocessingpipeline

cijothomas merged commit afd9135 into open-telemetry:master Jun 18, 2020

cijothomas deleted the cijothomas/activityprocessingpipeline branch June 18, 2020 00:43

This was referenced Aug 3, 2020

Need to rename BroadcastActivityProcessor #979

Closed

Renaming BroadcastActivity #1015

Merged

Conversation

cijothomas commented Jun 17, 2020

Changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjanotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas commented Jun 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants