Add ForceFlush to TracerProvider by kipwoker · Pull Request #1837 · open-telemetry/opentelemetry-dotnet

kipwoker · 2021-02-17T09:10:14Z

I'm using OpenTelemetry for AWS lambda functions. And I'm using dotnet opentelemetry library to achieve that. After each call, lambda goes to sleep and freezes background threads. Therefore I need to send activities to the exporter forcibly if I want to use the batch processor instead of the simple one.

Thankfully, BaseProcessor already contains a ForceFlush method that works perfectly in this case.

I've made a proof of concept

public static void ForceFlush(this TracerProvider tracerProvider)
{
	var tracerProviderSdkType = typeof(BaseProcessor<>).Assembly.GetType("OpenTelemetry.Trace.TracerProviderSdk");
	var tracerProviderSdk = tracerProvider.Cast(tracerProviderSdkType);
	var processorObj = tracerProviderSdk.GetFieldValue("processor");
	var processor = (BaseProcessor<Activity>) processorObj;
	processor.ForceFlush();
}

and it works as I expected. The only problem — access modifiers. This PR allows the user to trigger TracerProvider.ForceFlush without reflection. As I can see java solution has the same feature.

Related to: open-telemetry/opentelemetry-java#1068

linux-foundation-easycla · 2021-02-17T09:10:17Z

The committers are authorized under a signed CLA.

✅ Kirill Ivanov (5085a32865a85f559f89d5e23830b7a86563a5f6)

utpilla · 2021-02-18T00:21:31Z

If you want to do a ForceFlush on your BatchExporter, you could store a reference to the BatchExportProcessor, pass it AddProcessor method when building the TracerProvider, and call ForceFlush on it when needed. You could try something like this:

var exporter = new ConsoleActivityExporter(new ConsoleExporterOptions());
var processor = new BatchActivityExportProcessor(exporter); // store a reference to the BatchExportProcessor so that you can call ForceFlush on it later on
var tracerProvider = Sdk.CreateTracerProviderBuilder()
       .SetSampler(new AlwaysOnSampler())
       .AddSource("MyCompany.MyProduct.MyLibrary")
       .AddProcessor(processor) // Pass the reference to the BatchExportProcessor here
       .Build();

// Create activities

processor.ForceFlush(); // Call ForceFlush on the BatchExportProcessor

ForceFlush is not meant for TracerProvider. According to the spec, it is only provided for Processors and is relevant only to BatchExportProcessors.

reyang · 2021-02-18T00:24:17Z

The latest link of OpenTelemetry Java SdkTracerProvider.forceFlush is here.

I guess we want to test the water if this is something that should be added in the spec?

kipwoker · 2021-02-18T08:16:26Z

If you want to do a ForceFlush on your BatchExporter, you could store a reference to the BatchExportProcessor, pass it AddProcessor method when building the TracerProvider, and call ForceFlush on it when needed. You could try something like this:
var exporter = new ConsoleActivityExporter(new ConsoleExporterOptions());
var processor = new BatchActivityExportProcessor(exporter); // store a reference to the BatchExportProcessor so that you can call ForceFlush on it later on
var tracerProvider = Sdk.CreateTracerProviderBuilder()
       .SetSampler(new AlwaysOnSampler())
       .AddSource("MyCompany.MyProduct.MyLibrary")
       .AddProcessor(processor) // Pass the reference to the BatchExportProcessor here
       .Build();

// Create activities

processor.ForceFlush(); // Call ForceFlush on the BatchExportProcessor
ForceFlush is not meant for TracerProvider. According to the spec, it is only provided for Processors and is relevant only to BatchExportProcessors.

@utpilla I thought about this approach, but if you have a look at exporters none of them provides such a feature.
Examples:

ForceFlush is not meant for TracerProvider. According to the spec, it is only provided for Processors and is relevant only to BatchExportProcessors.

The same you can say about Shutdown method, but there is an extension TracerProviderExtensions that allows to do it, isn't it?
And, honestly, semantically ForceFlush is a "subset" of Shudown according to spec you linked:

Shutdown MUST include the effects of ForceFlush.

So my question is:
Why having Shutdown method is okay and what wrong could happen if we add ForceFlush?

kipwoker · 2021-02-18T08:19:01Z

The latest link of OpenTelemetry Java SdkTracerProvider.forceFlush is here.

I guess we want to test the water if this is something that should be added in the spec?

@reyang thank you, fixed the link :)
I didn't get your question. What exactly you want to test? :)

reyang · 2021-02-18T08:30:31Z

I didn't get your question. What exactly you want to test? :)

If you check the issue template, it mentioned:

Before opening a feature request against this repo, consider whether the feature should/could be implemented in the other OpenTelemetry client libraries. If so, please open an issue on opentelemetry-specification first.

What I want to test: if the spec is okay to take the change and add TracerProvider.ForceFlush.

kipwoker · 2021-02-18T08:52:33Z

Also, found the java PR. The conversation there may be useful. The author of this PR had the same issue with the lambda, so probably people will want to have something like this feature.

reyang · 2021-02-18T08:57:55Z

This could throw NullReferenceException if this.processor is null.

reyang

The change and the rationale make sense to me.
LGTM assuming the NullReferenceException comment is resolved.

reyang · 2021-02-18T09:04:18Z

Consider adding a small test case just to prevent the regression.

reyang · 2021-02-18T09:04:55Z

Consider updating the CHANGELOG.

kipwoker · 2021-02-18T10:44:21Z

@reyang thank you, good points. Everything is fixed. Please, check it's correct.

codecov · 2021-02-18T16:04:40Z

Codecov Report

Merging #1837 (9e078fe) into main (fe24768) will increase coverage by 0.91%.
The diff coverage is 67.81%.

@@            Coverage Diff             @@
##             main    #1837      +/-   ##
==========================================
+ Coverage   82.88%   83.79%   +0.91%     
==========================================
  Files         193      187       -6     
  Lines        6005     5967      -38     
==========================================
+ Hits         4977     5000      +23     
+ Misses       1028      967      -61

Impacted Files	Coverage Δ
...nTelemetry.Exporter.Jaeger/Implementation/Batch.cs	`84.09% <ø> (+3.65%)`	⬆️
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs	`15.38% <ø> (ø)`
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs	`100.00% <ø> (ø)`
...metryProtocol/OtlpTraceExporterHelperExtensions.cs	`92.30% <ø> (ø)`
....Exporter.Zipkin/ZipkinExporterHelperExtensions.cs	`15.38% <ø> (ø)`
...Telemetry.Exporter.Zipkin/ZipkinExporterOptions.cs	`100.00% <ø> (ø)`
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	`47.22% <40.62%> (-10.23%)`	⬇️
...rc/OpenTelemetry/Trace/TracerProviderExtensions.cs	`32.25% <45.45%> (+7.25%)`	⬆️
...rc/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs	`85.00% <84.21%> (-0.27%)`	⬇️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	`100.00% <100.00%> (ø)`
... and 12 more

reyang · 2021-02-18T16:30:11Z

+        /// Thrown when the <c>timeoutMilliseconds</c> is smaller than -1.
+        /// </exception>
+        /// <remarks>
+        /// This function guarantees thread-safety. Only the first call will


opentelemetry-dotnet/src/OpenTelemetry/BaseProcessor.cs

Line 81 in c42ce19

/// This function guarantees thread-safety.

The semantic is a bit different than Shutdown, ForceFlush can be called multiple times and there is no restriction on which call will win (it is a win-win situation😄).

Consider borrow the wording from

opentelemetry-dotnet/src/OpenTelemetry/BaseProcessor.cs

Line 67 in c42ce19

/// Flushes the processor, blocks the current thread until flush

.

Oops, copypaste issue :D
I will change according to this doc: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/BaseProcessor.cs#L81

Consider borrow the wording from

Thank you. Took this part and also added information about TracerProviderSdk like it was made on the Shutdown method.

Looks perfect to me now.

utpilla · 2021-02-18T19:35:07Z

If you want to do a ForceFlush on your BatchExporter, you could store a reference to the BatchExportProcessor, pass it AddProcessor method when building the TracerProvider, and call ForceFlush on it when needed. You could try something like this:
var exporter = new ConsoleActivityExporter(new ConsoleExporterOptions());
var processor = new BatchActivityExportProcessor(exporter); // store a reference to the BatchExportProcessor so that you can call ForceFlush on it later on
var tracerProvider = Sdk.CreateTracerProviderBuilder()
       .SetSampler(new AlwaysOnSampler())
       .AddSource("MyCompany.MyProduct.MyLibrary")
       .AddProcessor(processor) // Pass the reference to the BatchExportProcessor here
       .Build();

// Create activities

processor.ForceFlush(); // Call ForceFlush on the BatchExportProcessor
ForceFlush is not meant for TracerProvider. According to the spec, it is only provided for Processors and is relevant only to BatchExportProcessors.
@utpilla I thought about this approach, but if you have a look at exporters none of them provides such a feature.
Examples:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs

You can achieve the same for Zipkin or Jaeger or any other exporter using the same approach:

var exporter = new JaegerExporter(new JaegerExporterOptions());
var processor = new BatchActivityExportProcessor(exporter);

.....
processor.ForceFlush();

ForceFlush is not meant for TracerProvider. According to the spec, it is only provided for Processors and is relevant only to BatchExportProcessors.

The same you can say about Shutdown method, but there is an extension TracerProviderExtensions that allows to do it, isn't it?

No the spec explicitly asks to provide a Shutdown method for TracerProvider. Here is the link to that: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#shutdown.

And, honestly, semantically ForceFlush is a "subset" of Shudown according to spec you linked:

Shutdown MUST include the effects of ForceFlush.

"Shutdown MUST include the effects of ForceFlush." is applicable to the Shutdown of Span Processors. Also, if you look at the requirements for TracerProvider Shutdown it says that: Shutdown MUST be implemented at least by invoking Shutdown within all internal processors. So, in effect, if you call the Shutdown on the TracerProvider, it will call the Shutdown on all of its processor which include the effects of a ForceFlush.

So my question is:
Why having Shutdown method is okay and what wrong could happen if we add ForceFlush?

I have just been stating why we have the TracerProvider and Processors implemented the way they are. Having said that, if we feel that we need to add ForceFlush to TracerProvider, we should go forward with it and also update the spec.

cijothomas · 2021-02-18T21:59:54Z

@kipwoker Thanks for the PR.
Could you open an issue in the spec repo to have this requirement added to the spec itself? While the feature itself is good, we generally try to limit adding features (esp. ones require public API change), unless this is a spec requirement, or a .NET specific requirement.

In this case, this issue is not limited to .NET or Java, so please open a spec issue.
And please confirm if holding reference to processor, and calling ForceFlush() on it works for you or not.

cijothomas · 2021-02-22T18:08:41Z

open-telemetry/opentelemetry-specification#1452 is merged

kipwoker · 2021-02-23T08:09:54Z

open-telemetry/opentelemetry-specification#1452 is merged

For me, it seems done and ready to merge.
Feel free to ask if you need anything else from me related to this PR. :)

Oberon00 · 2021-02-23T08:47:07Z

+                throw new ArgumentNullException(nameof(provider));
+            }
+
+            if (provider is TracerProviderSdk tracerProviderSdk)


This seems like a hack. There should be some public type (class or interface) representing TracerProviderSdk. But I guess it's too late for that 😃

Agree. :)
I just made it like it was done for Shutdown method. In my opinion, it's better to change nothing at the moment and make architectural changes at separate PR.

Lets not address any other issue in this PR :)
Happy to address them separate. (or not address :D)

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

kipwoker requested a review from a team February 17, 2021 09:10

kipwoker changed the title ~~Added ForceFlush to TracerProvider~~ Add ForceFlush to TracerProvider Feb 17, 2021

reyang reviewed Feb 18, 2021

View reviewed changes

reyang approved these changes Feb 18, 2021

View reviewed changes

reyang reviewed Feb 18, 2021

View reviewed changes

Comment thread src/OpenTelemetry/Trace/TracerProviderExtensions.cs Outdated

Add ForceFlush to TracerProvider

8c6a7c2

reyang reviewed Feb 18, 2021

View reviewed changes

Kirill Ivanov added 2 commits February 18, 2021 17:34

Fixed inlinedoc

442d9dd

Change the wording

1908ed9

reyang added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package enhancement New feature or request labels Feb 18, 2021

reyang added the needs-spec-change Issues which require the OpenTelemetry Specification to clarify or define behavior label Feb 19, 2021

kipwoker mentioned this pull request Feb 19, 2021

Add ForceFlush to Tracing SDK open-telemetry/opentelemetry-specification#1451

Closed

cijothomas reviewed Feb 19, 2021

View reviewed changes

Comment thread test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs Outdated

Kirill Ivanov and others added 2 commits February 22, 2021 09:58

Remove creating activity

e13cd50

Merge branch 'main' into forceflush-tracerprovider

64ea29d

cijothomas removed the needs-spec-change Issues which require the OpenTelemetry Specification to clarify or define behavior label Feb 22, 2021

Oberon00 reviewed Feb 23, 2021

View reviewed changes

cijothomas reviewed Feb 23, 2021

View reviewed changes

Comment thread src/OpenTelemetry/CHANGELOG.md Outdated

cijothomas approved these changes Feb 23, 2021

View reviewed changes

Update src/OpenTelemetry/CHANGELOG.md

aceb0d5

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

cijothomas merged commit cb066cb into open-telemetry:main Feb 23, 2021

kipwoker deleted the forceflush-tracerprovider branch February 24, 2021 07:42

Conversation

kipwoker commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

utpilla commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reyang commented Feb 18, 2021

Uh oh!

kipwoker commented Feb 18, 2021

Uh oh!

kipwoker commented Feb 18, 2021

Uh oh!

reyang commented Feb 18, 2021

Uh oh!

kipwoker commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reyang commented Feb 18, 2021

Uh oh!

reyang commented Feb 18, 2021

Uh oh!

kipwoker commented Feb 18, 2021

Uh oh!

codecov Bot commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

utpilla commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cijothomas commented Feb 18, 2021

Uh oh!

Uh oh!

cijothomas commented Feb 22, 2021

Uh oh!

kipwoker commented Feb 23, 2021

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kipwoker commented Feb 17, 2021 •

edited

Loading

linux-foundation-easycla Bot commented Feb 17, 2021 •

edited

Loading

utpilla commented Feb 18, 2021 •

edited

Loading

codecov Bot commented Feb 18, 2021 •

edited

Loading

utpilla commented Feb 18, 2021 •

edited

Loading