Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Jun 16, 2021

Conversation

kkruk-sumo
Copy link
Contributor

Which problem is this PR solving?

Fixes #1721

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.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kkruk-sumo
Copy link
Contributor Author

kkruk-sumo commented Jun 1, 2021

@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.

@kkruk-sumo kkruk-sumo requested a review from obecny June 1, 2021 15:24
@@ -190,4 +195,7 @@ export class BatchSpanProcessor implements SpanProcessor {
this._timer = undefined;
}
}

abstract onInit(config?: T): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/opentelemetry-tracing/src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kkruk-sumo kkruk-sumo requested a review from MSNev as a code owner June 8, 2021 09:18
@kkruk-sumo kkruk-sumo requested a review from dyladan June 8, 2021 10:50
Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart enough to be called twice concurrently you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kkruk-sumo kkruk-sumo requested a review from johnbley June 8, 2021 12:34
Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks.

@dyladan dyladan added the enhancement New feature or request label Jun 8, 2021
@codecov
Copy link

codecov bot commented Jun 8, 2021

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%> (ø)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please rebase so I can merge it

@dyladan
Copy link
Member

dyladan commented Jun 10, 2021

@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
Copy link
Member

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

@kkruk-sumo
Copy link
Contributor Author

kkruk-sumo commented Jun 15, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send batched export items in when visibilityState changes to hidden
6 participants