feat(tracing): auto flush BatchSpanProcessor on browser #2243

kkruk-sumo · 2021-05-31T16:28:09Z

Which problem is this PR solving?

After this PR BatchSpanProcessor will automatically flush spans when browser page become hidden (by closing it, changing tab, closing browser on mobile).
This change doesn't guarantee that these spans will be sent. It depends on the exporter implementation, especially usage of navigator.sendBeacon which can be called to perform HTTP requests before page unload (exporter-collector uses sendBeacon when no headers are specified).

Short description of the changes

Current BatchSpanProcessor class is now abstract. Each platform (node and browser) now have custom BatchSpanProcessor classes.
It's a first platform-specific code in @sumologin/tracing so I had to slightly reorganize files.

Fixes open-telemetry#1721

obecny

In general I like the idea very much, I raised few issues / concerns.

You are missing browser property in package.json with 3 entries, check for example collector exporter

packages/opentelemetry-tracing/src/platform/browser/export/BatchSpanProcessor.ts

packages/opentelemetry-tracing/test/browser/export/BatchSpanProcessor.test.ts

kkruk-sumo · 2021-06-01T14:22:50Z

@obecny

You are missing browser property in package.json with 3 entries, check for example collector exporter

There is "browser" field in package.json. It looks the same as the one in exporter-collector.

dyladan · 2021-06-01T19:15:12Z

packages/opentelemetry-tracing/src/export/BatchSpanProcessorBase.ts

@@ -190,4 +195,7 @@ export class BatchSpanProcessor implements SpanProcessor {
      this._timer = undefined;
    }
  }
+
+  abstract onInit(config?: T): void;


Why make these abstract if they're only used by browser? Wouldn't it make more sense to just override the constructor in the browser impl then call onInit after super?

packages/opentelemetry-tracing/src/types.ts

obecny

lgtm

…r-auto-flush

…ntHide

johnbley

I like the idea; one small suggestion for better cross-browser support.

johnbley · 2021-06-08T12:09:55Z

packages/opentelemetry-tracing/src/platform/browser/export/BatchSpanProcessor.ts

+
+  private onInit(config?: BatchSpanProcessorBrowserConfig): void {
+    if (config?.disableAutoFlushOnDocumentHide !== true && document != null) {
+      this._visibilityChangeListener = () => {


This should also trigger flush on a pagehide event - Safari doesn't fully support visibilitychange: https://caniuse.com/?search=visibilitychange

Good catch. I added support for the pagehide event.
I didn't add any check to prevent double call of forceFlush when both pagehide and visibilitychange events are dispatched to make the code simpler. Let me know if you want such mechanism.

I don't think there's any need for that level of complexity - flush implementations should be smart enough.

smart enough to be called twice concurrently you mean?

There is also beforeunload and unload (depending on the browser support matrix that we want to support -- specifically older IE instances (which are probably already excluded by other usages)) that you may want to also consider adding.

Additionally, there is actually a set of corner cases that we should support around pages hooking theses same events after the SDK has initialized but still creating / sending telemetry. The simplest solution for this is to set a flag during these flush events (or more specifically the unload events) and then during the onEnd() as well as batching you also trigger the same auto flush.

I described my concerns about using unload event here #2205 .

Also I was testing JS auto-instrumentation on different browsers and on IE11 I got a Script error so I guess we don't support it.

This PR is about auto flushing created spans when page become hidden, so I don't think there is something wrong with sending telemetry when page is not active, or I didn't get your point right?

-1 for unload: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon has a full writeup of the recommended approach. What's in the PR now looks good.

If the current BatchSpanExporter.forceFlush isn't smart enough to deal with two calls in succession, I think it should be improved as a separate PR rather than complicating state/logic here. A surface skim looks like it might have some issues...

My comment is not a blocking comment for this PR -- as @johnbley says What's in the PR now looks good as it's making this better

johnbley

lgtm, thanks.

codecov · 2021-06-08T17:24:57Z

Codecov Report

Merging #2243 (8e04a73) into main (a3b7738) will decrease coverage by 0.12%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##             main    #2243      +/-   ##
==========================================
- Coverage   92.83%   92.70%   -0.13%     
==========================================
  Files         142      145       +3     
  Lines        5094     5184      +90     
  Branches     1042     1060      +18     
==========================================
+ Hits         4729     4806      +77     
- Misses        365      378      +13

Impacted Files	Coverage Δ
...ing/src/platform/node/export/BatchSpanProcessor.ts	`66.66% <66.66%> (ø)`
...metry-tracing/src/export/BatchSpanProcessorBase.ts	`91.39% <85.71%> (ø)`
...s/opentelemetry-tracing/src/BasicTracerProvider.ts	`94.49% <100.00%> (ø)`
...es/opentelemetry-context-zone-peer-dep/src/util.ts	`100.00% <0.00%> (ø)`
...ry-context-zone-peer-dep/src/ZoneContextManager.ts	`85.18% <0.00%> (ø)`

dyladan

LGTM. Please rebase so I can merge it

dyladan · 2021-06-10T19:30:55Z

@kkruk-sumo can you please check the box to allow maintainers to edit your branch? Otherwise it is difficult for me to merge PRs since they have to be up to date with the main branch

vmarchaud · 2021-06-12T08:19:24Z

@kkruk-sumo Sorry but you'll need to rebase again

kkruk-sumo · 2021-06-15T11:26:17Z

@dyladan Next time I will create PR's from my local fork. Right now I don't have permissions to allow maintainers to edit my branch. Sorry.

…ry#2243)

feat(tracing): auto flush BatchSpanProcessor on browser

e62100e

Fixes open-telemetry#1721

kkruk-sumo requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners May 31, 2021 16:28

kkruk-sumo mentioned this pull request May 31, 2021

Send batched export items in when visibilityState changes to hidden #1721

Closed

2 tasks

obecny reviewed May 31, 2021

View reviewed changes

packages/opentelemetry-tracing/src/platform/browser/export/BatchSpanProcessor.ts Outdated Show resolved Hide resolved

packages/opentelemetry-tracing/test/browser/export/BatchSpanProcessor.test.ts Outdated Show resolved Hide resolved

kkruk-sumo added 3 commits June 1, 2021 17:15

feat: added flushOnDocumentBecomesHidden config to BatchSpanProcessor

1008e03

chore(tracing): don't run browser tests for node

2386651

chore: added test for flushOnDocumentBecomesHidden option being true

978ea0e

kkruk-sumo requested a review from obecny June 1, 2021 15:24

dyladan reviewed Jun 1, 2021

View reviewed changes

obecny approved these changes Jun 3, 2021

View reviewed changes

kkruk-sumo added 4 commits June 8, 2021 10:10

Merge remote-tracking branch 'upstream/main' into batch-span-processo…

a92a5f8

…r-auto-flush

feat: rename flushOnDocumentBecomesHidden to disableAutoFlushOnDocume…

6f22408

…ntHide

chore: added trailing newline

ada0658

chore: moved onInit out of BatchSpanProcessorBase

9b17a3c

kkruk-sumo requested a review from MSNev as a code owner June 8, 2021 09:18

kkruk-sumo requested a review from dyladan June 8, 2021 10:50

johnbley suggested changes Jun 8, 2021

View reviewed changes

feat: added support for pagehide event

3f6d327

kkruk-sumo requested a review from johnbley June 8, 2021 12:34

johnbley approved these changes Jun 8, 2021

View reviewed changes

dyladan added the enhancement New feature or request label Jun 8, 2021

Merge branch 'main' into batch-span-processor-auto-flush

4093ec9

dyladan approved these changes Jun 9, 2021

View reviewed changes

Merge branch 'main' into batch-span-processor-auto-flush

c7ba6bf

Merge branch 'main' into batch-span-processor-auto-flush

23038bd

MSNev approved these changes Jun 11, 2021

View reviewed changes

Merge branch 'main' into batch-span-processor-auto-flush

8e04a73

dyladan merged commit 23db7f0 into open-telemetry:main Jun 16, 2021

kkruk-sumo deleted the batch-span-processor-auto-flush branch June 16, 2021 13:37

dyladan mentioned this pull request Jun 16, 2021

chore: release proposal 0.22.0 #2288

Merged

kkruk-sumo added a commit to SumoLogic/opentelemetry-js that referenced this pull request Aug 27, 2021

feat(tracing): auto flush BatchSpanProcessor on browser (open-telemet…

0a7225e

…ry#2243)

kkruk-sumo added a commit to SumoLogic/opentelemetry-js that referenced this pull request Sep 20, 2021

feat(tracing): auto flush BatchSpanProcessor on browser (open-telemet…

b25e1bd

…ry#2243)

legendecas mentioned this pull request Jun 14, 2022

Auto shutdown in collector-exporter can hurt performance in some browsers #2205

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracing): auto flush BatchSpanProcessor on browser #2243

feat(tracing): auto flush BatchSpanProcessor on browser #2243

kkruk-sumo commented May 31, 2021

obecny left a comment

kkruk-sumo commented Jun 1, 2021 •

edited

Loading

dyladan Jun 1, 2021

kkruk-sumo Jun 8, 2021

obecny left a comment

johnbley left a comment

johnbley Jun 8, 2021

kkruk-sumo Jun 8, 2021

johnbley Jun 8, 2021

dyladan Jun 8, 2021

MSNev Jun 8, 2021

kkruk-sumo Jun 9, 2021

johnbley Jun 9, 2021

MSNev Jun 11, 2021

johnbley left a comment

codecov bot commented Jun 8, 2021 •

edited

Loading

dyladan left a comment

dyladan commented Jun 10, 2021

vmarchaud commented Jun 12, 2021

kkruk-sumo commented Jun 15, 2021 •

edited

Loading

feat(tracing): auto flush BatchSpanProcessor on browser #2243

feat(tracing): auto flush BatchSpanProcessor on browser #2243

Conversation

kkruk-sumo commented May 31, 2021

Which problem is this PR solving?

Short description of the changes

obecny left a comment

Choose a reason for hiding this comment

kkruk-sumo commented Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

johnbley 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbley left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 8, 2021 • edited Loading

Codecov Report

dyladan left a comment

Choose a reason for hiding this comment

dyladan commented Jun 10, 2021

vmarchaud commented Jun 12, 2021

kkruk-sumo commented Jun 15, 2021 • edited Loading

kkruk-sumo commented Jun 1, 2021 •

edited

Loading

codecov bot commented Jun 8, 2021 •

edited

Loading

kkruk-sumo commented Jun 15, 2021 •

edited

Loading