feat(opentelemetry-sdk-trace-base): Add mandatory forceFlush property to SpanExporter interface #3071

sgracias1 · 2022-06-29T20:23:57Z

Which problem is this PR solving?

Add optional forceFlush property to SpanExporter interface per spec

Fixes #3067

Short description of the changes

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

added unit tests

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

…to SpanExporter interface Signed-off-by: Sidartha Gracias <[email protected]>

Signed-off-by: Sidartha Gracias <[email protected]>

linux-foundation-easycla · 2022-06-29T20:24:02Z

The committers listed above are authorized under a signed CLA.

✅ login: sgracias1 / name: Sidartha Gracias (01fe26e, 9073b2c, a8bc10d)

legendecas · 2022-06-30T02:49:14Z

Thank you for your contribution! I think built-in SpanProcessors like BatchSpanProcessor and SimpleSpanProcessor need to adopt the method and invoke the exporter's forceFlush if it exists. Also, would you mind adding some test cases covering the method?

sgracias1 · 2022-06-30T16:05:27Z

I see thank you, let me look at the span processors. For the test cases, do mean for testing span processors once the change is made?

dyladan · 2022-06-30T16:08:40Z

I see thank you, let me look at the span processors. For the test cases, do mean for testing span processors once the change is made?

Yes the span processors will need to be tested. There really is no other way to test an interface than to test its implementers.

codecov · 2022-06-30T16:11:43Z

Codecov Report

Merging #3071 (6c8b184) into main (34c5bdb) will increase coverage by 0.02%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #3071      +/-   ##
==========================================
+ Coverage   93.18%   93.21%   +0.02%     
==========================================
  Files         196      196              
  Lines        6431     6440       +9     
  Branches     1359     1359              
==========================================
+ Hits         5993     6003      +10     
+ Misses        438      437       -1

Impacted Files	Coverage Δ
...y-sdk-trace-base/src/export/ConsoleSpanExporter.ts	`81.25% <50.00%> (+2.67%)`	⬆️
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts	`95.23% <100.00%> (+0.11%)`	⬆️
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts	`94.80% <100.00%> (+0.13%)`	⬆️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	`100.00% <100.00%> (ø)`
...-sdk-trace-base/src/export/InMemorySpanExporter.ts	`100.00% <100.00%> (ø)`
...y-sdk-trace-base/src/export/SimpleSpanProcessor.ts	`91.30% <100.00%> (ø)`
...-trace-base/src/platform/node/RandomIdGenerator.ts	`93.75% <0.00%> (+6.25%)`	⬆️

sgracias1 · 2022-07-12T21:08:42Z

@legendecas @dyladan
Since we are updating the spec to make forceFlush mandatory what do you think the implementation should look like
Looking at the implementation (ConsoleSpanExporter, InMemorySpanExporter) it seems like it would duplicate what the current shutdown function does or should be a no-op ?

Also looking at BatchSpanProcessorBase, it currently implements forceFlush to call export for each batch, it doesn't seem to make sense to invoke the forceFlush on the exporter (since there shouldn't be anything left to export)

dyladan · 2022-07-14T02:26:12Z

Force flush in the exporter should return a promise which resolves when the currently export completes. If there is no batch currently exporting then it immediately resolves.

Signed-off-by: Sidartha Gracias <[email protected]>

… for downstream exporters Signed-off-by: Sidartha Gracias <[email protected]>

sgracias1 · 2022-07-27T21:45:41Z

BatchSpanProcessorBase already implements forceFlush. I have not changed it. Should we leave it as is or re-implement that functionality to invoke the exporters forceFlush function?

Making the forceFlush function mandatory required updating a few downstream exporters

JaegerExporter
ZipkinExporter
OTLPExporterBase

I have given these empty implementations for now, does that make sense? I spent a little time looking at these classes but it wasn't obvious to me what the implementation of forceFlush should be for these classes.

dyladan

Please change the title as the interface is no longer optional

experimental/packages/otlp-exporter-base/src/OTLPExporterBase.ts

dyladan · 2022-07-28T17:35:24Z

packages/opentelemetry-exporter-jaeger/src/jaeger.ts

@@ -82,6 +82,10 @@ export class JaegerExporter implements SpanExporter {
 return this._shutdownOnce.call();
 }

+ forceFlush(): Promise<void> {
+ return Promise.resolve();


Same as OTLP if it is possible then ForceFlush should only resolve when any in-flight exports are complete.

dyladan · 2022-07-28T17:35:30Z

packages/opentelemetry-exporter-zipkin/src/zipkin.ts

@@ -109,6 +109,10 @@ export class ZipkinExporter implements SpanExporter {
 });
 }

+ forceFlush(): Promise<void> {
+ return Promise.resolve();


packages/opentelemetry-sdk-trace-base/src/export/ConsoleSpanExporter.ts

dyladan · 2022-07-28T17:36:48Z

packages/opentelemetry-sdk-trace-base/src/export/InMemorySpanExporter.ts

+ return this._flush();
+ }
+
+ private _flush(): Promise<void> {


Why have a separate _flush method? Can't shutdown just call ForceFlush?

dyladan · 2022-07-28T17:36:59Z

packages/opentelemetry-sdk-trace-base/src/export/InMemorySpanExporter.ts

+ return this._flush();
+ }
+
+ private _flush(): Promise<void> {
 this._finishedSpans = [];


Flushing should not reset. There is a dedicated method for that

@dyladan thank you for the review, can you give your thoughts on BatchSpanProcessorBase (I posted a comment above) should we reimplement it, or leave as is

dyladan · 2022-07-29T16:17:22Z

The batch span processor is specified to not call the exporter multiple times in parallel so you should be able to use forceflush to make sure the export is finished before the next batch is sent.

… for zipkin, jaeger, otlp

dyladan · 2022-08-01T20:20:24Z

experimental/packages/otlp-exporter-base/src/OTLPExporterBase.ts

+ .then(() => {
+ /** ignore resolved values */
+ });


No need to do .then if you're just going to ignore the value.

I can remove this, it was mostly a refactor (I moved the code out of the current shutdown function

ah i see. i would just remove it since it doesn't serve a purpose

Okay I will wrap this in a new promise, returning promise.all wouldn't work with the function type returning a promise

sgracias1 · 2022-08-01T20:34:00Z

@dyladan

The batch span processor is specified to not call the exporter multiple times in parallel so you should be able to use forceflush to make sure the export is finished before the next batch is sent.

the current behavior of forceFlush on the processor returns a promise which resolves when all spans have been exported, so we currently

wrap each export(batch) in a promise
promise.all(each export promise)

I don't really see where in this flow we would wan't to add a forceflush call. Do you think something like
1: export(batch)
2: forceflush
3: promise.all(each flush promise)

we do a bit of cleanup when each export completes clearTimeout that I don't quite understand, but it seems like we need to handle the return from each export as well.

dyladan · 2022-08-01T20:35:47Z

The force flush isn't meant to be used in most cases. It is for special situations like shutdown and when you are in an environment that might unexpectedly get frozen or stopped like lambda.

dyladan · 2022-08-02T18:46:05Z

BatchSpanProcessorBase already implements forceFlush. I have not changed it. Should we leave it as is or re-implement that functionality to invoke the exporters forceFlush function?

Making the forceFlush function mandatory required updating a few downstream exporters

JaegerExporter

ZipkinExporter

OTLPExporterBase

I have given these empty implementations for now, does that make sense? I spent a little time looking at these classes but it wasn't obvious to me what the implementation of forceFlush should be for these classes.

I think you can go either way but if you don't implement it please open an issue and we can get it fixed asap

sgracias1 · 2022-08-04T19:31:39Z

> I think you can go either way but if you don't implement it please open an issue and we can get it fixed asap

I have updated all the downstream exporters
I am leaving the BatchSpanProcessorBase as is, let me know if you think this needs another issue

legendecas

Thank you for your contribution. Overall LGTM % some nits.

packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts

packages/opentelemetry-sdk-trace-base/test/common/export/SimpleSpanProcessor.test.ts

packages/opentelemetry-sdk-trace-base/test/common/export/ConsoleSpanExporter.test.ts

packages/opentelemetry-exporter-zipkin/test/node/zipkin.test.ts

packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts

experimental/packages/otlp-exporter-base/test/node/util.test.ts

…ry-js into Issue_3067

sgracias1 · 2022-08-11T16:42:50Z

@legendecas thank you, let me know if the updated pr looks alright to you

experimental/packages/otlp-exporter-base/src/OTLPExporterBase.ts

sgracias1 · 2022-08-17T17:20:08Z

@dyladan can you let me know if this pr looks okay to you now

…ry-js into Issue_3067

github-actions · 2022-10-31T06:41:29Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions · 2023-01-30T06:36:18Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

legendecas · 2023-01-31T06:55:06Z

@dyladan another look at this is appreciated :)

JacksonWeber · 2023-03-13T20:56:22Z

@sgracias1 Would it be possible for me to pull down your PR and make the appropriate suggested changes?

sgracias1 added 3 commits June 29, 2022 12:40

feat(opentelemetry-sdk-trace-base): Add optional forceFlush property …

01fe26e

…to SpanExporter interface Signed-off-by: Sidartha Gracias <[email protected]>

feat(opentelemetry-sdk-trace-base): fixup changelog

9073b2c

Signed-off-by: Sidartha Gracias <[email protected]>

feat(opentelemetry-sdk-trace-base): fixup changelog

a8bc10d

Signed-off-by: Sidartha Gracias <[email protected]>

sgracias1 requested a review from a team June 29, 2022 20:23

sgracias1 and others added 3 commits July 27, 2022 12:30

Merge branch 'main' into Issue_3067

f746ece

feat(opentelemetry-sdk-trace-base): add exporter forceflush functions

c2af065

Signed-off-by: Sidartha Gracias <[email protected]>

feat(opentelemetry-sdk-trace-base): add tests, add empty implemtation…

55c5c36

… for downstream exporters Signed-off-by: Sidartha Gracias <[email protected]>

dyladan reviewed Jul 28, 2022

View reviewed changes

sgracias1 changed the title ~~feat(opentelemetry-sdk-trace-base): Add optional forceFlush property to SpanExporter interface~~ feat(opentelemetry-sdk-trace-base): Add mandatory forceFlush property to SpanExporter interface Jul 29, 2022

sgracias1 added 2 commits August 1, 2022 12:23

feat(opentelemetry-sdk-trace-base): add implementation for forceflush…

18e90f5

… for zipkin, jaeger, otlp

Merge remote-tracking branch 'upstream/main' into Issue_3067

99c1be5

dyladan reviewed Aug 1, 2022

View reviewed changes

sgracias1 and others added 4 commits August 4, 2022 12:32

feat(opentelemetry-sdk-trace-base): fix lint, minor review change

8058f45

Merge remote-tracking branch 'upstream/main' into Issue_3067

5b7dce7

Merge branch 'main' into Issue_3067

4bc295a

Merge branch 'main' into Issue_3067

b09a48f

legendecas reviewed Aug 9, 2022

View reviewed changes

sgracias1 added 3 commits August 11, 2022 09:39

feat(opentelemetry-sdk-trace-base): minor review change

39160d7

Merge remote-tracking branch 'upstream/main' into Issue_3067

f7b49fb

Merge branch 'Issue_3067' of https:/sgracias1/opentelemet…

9eae0d4

…ry-js into Issue_3067

feat(opentelemetry-sdk-trace-base): fix lint

3490194

legendecas approved these changes Aug 15, 2022

View reviewed changes

experimental/packages/otlp-exporter-base/src/OTLPExporterBase.ts Outdated Show resolved Hide resolved

Merge branch 'main' into Issue_3067

9d7b74e

sgracias1 added 2 commits August 18, 2022 09:47

feat(opentelemetry-sdk-trace-base): minor change

5a81e5f

Merge branch 'Issue_3067' of https:/sgracias1/opentelemet…

6c8b184

…ry-js into Issue_3067

github-actions bot added the stale label Oct 31, 2022

dyladan removed the stale label Nov 2, 2022

github-actions bot added the stale label Jan 30, 2023

legendecas added sdk:traces Issues and PRs related to the Traces SDK and removed stale labels Jan 31, 2023

JacksonWeber mentioned this pull request Mar 9, 2023

Implement SpanExporter#forceFlush #3067

Closed

pichlermarc closed this May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opentelemetry-sdk-trace-base): Add mandatory forceFlush property to SpanExporter interface #3071

feat(opentelemetry-sdk-trace-base): Add mandatory forceFlush property to SpanExporter interface #3071

sgracias1 commented Jun 29, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jun 29, 2022 •

edited

Loading

legendecas commented Jun 30, 2022 •

edited

Loading

sgracias1 commented Jun 30, 2022

dyladan commented Jun 30, 2022

codecov bot commented Jun 30, 2022 •

edited

Loading

sgracias1 commented Jul 12, 2022

dyladan commented Jul 14, 2022

sgracias1 commented Jul 27, 2022

dyladan left a comment

dyladan Jul 28, 2022

dyladan Jul 28, 2022

dyladan Jul 28, 2022

dyladan Jul 28, 2022

sgracias1 Jul 29, 2022

dyladan commented Jul 29, 2022

dyladan Aug 1, 2022

sgracias1 Aug 1, 2022

dyladan Aug 2, 2022

sgracias1 Aug 4, 2022

sgracias1 commented Aug 1, 2022

dyladan commented Aug 1, 2022

dyladan commented Aug 2, 2022

sgracias1 commented Aug 4, 2022 •

edited

Loading

legendecas left a comment

sgracias1 commented Aug 11, 2022

sgracias1 commented Aug 17, 2022

github-actions bot commented Oct 31, 2022

github-actions bot commented Jan 30, 2023

legendecas commented Jan 31, 2023

JacksonWeber commented Mar 13, 2023

feat(opentelemetry-sdk-trace-base): Add mandatory forceFlush property to SpanExporter interface #3071

feat(opentelemetry-sdk-trace-base): Add mandatory forceFlush property to SpanExporter interface #3071

Conversation

sgracias1 commented Jun 29, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Jun 29, 2022 • edited Loading

legendecas commented Jun 30, 2022 • edited Loading

sgracias1 commented Jun 30, 2022

dyladan commented Jun 30, 2022

codecov bot commented Jun 30, 2022 • edited Loading

Codecov Report

sgracias1 commented Jul 12, 2022

dyladan commented Jul 14, 2022

sgracias1 commented Jul 27, 2022

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgracias1 commented Aug 1, 2022

dyladan commented Aug 1, 2022

dyladan commented Aug 2, 2022

sgracias1 commented Aug 4, 2022 • edited Loading

legendecas left a comment

Choose a reason for hiding this comment

sgracias1 commented Aug 11, 2022

sgracias1 commented Aug 17, 2022

github-actions bot commented Oct 31, 2022

github-actions bot commented Jan 30, 2023

legendecas commented Jan 31, 2023

JacksonWeber commented Mar 13, 2023

sgracias1 commented Jun 29, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jun 29, 2022 •

edited

Loading

legendecas commented Jun 30, 2022 •

edited

Loading

codecov bot commented Jun 30, 2022 •

edited

Loading

sgracias1 commented Aug 4, 2022 •

edited

Loading