feat: allow events other than 'unload' to be configured to trigger the shutdown procedure in OTLPExporterBase

[eldavojohn]

Which problem is this PR solving?

Google is planning to deprecate unload. This also currently blocks sites that use OTEL from benefiting from back/forward cache.

This PR proposes allowing the exporterConfig to possibly define whatever event they want and gives a union type of suggestions to be used (of course this can be overridden by tsignore if they choose).

Fixes #2205

Short description of the changes

As described in this comment: #4438 (comment) I have made this property an optional array. If it is not present, it will only assign the unload event.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Test A

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: eldavojohn (83841a8, 79b7415, 2a31c99, 1287f2d, 7b11f90, e493353, a7a6119, f8be66e, 9e89b0f, 4a1eee3, 1be7c91, a9ab63e, 7c0fd76, e99a273)

[eldavojohn]

@dyladan is there a procedure for getting comments/reviews on PRs here?

[dyladan]

Not really. Joining the SIG meeting on wednesdays is a good idea if you have the time in order to explain the issue you're trying to solve and how your solution works to potential reviewers. Web-specific changes also tend to be reviewed by different people than node-focused things, and at least one of them has been on vacation for the last week or two.

[codecov]

Codecov Report

Merging #4326 (e493353) into main (f86251d) will increase coverage by 2.49%.
The diff coverage is 100.00%.

❗ Current head e493353 differs from pull request most recent head e99a273. Consider uploading reports for the commit e99a273 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4326      +/-   ##
==========================================
+ Coverage   89.72%   92.21%   +2.49%     
==========================================
  Files         164      333     +169     
  Lines        3309     9460    +6151     
  Branches      692     2010    +1318     
==========================================
+ Hits         2969     8724    +5755     
- Misses        340      736     +396     

Files	Coverage Δ
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts	95.34% <100.00%> (+0.11%)	⬆️
...erimental/packages/otlp-exporter-base/src/types.ts	16.66% <ø> (ø)

... and 188 files with indirect coverage changes

[dyladan]

Looks like the lint is failing.

I looked into the linked article about the deprecation of unload. Is the reason to leave it as the default event to avoid the breaking change? If unload is going to be deprecated, it might be a good idea to change the default event, at least in the SDK 2.0 which is being currently planned and implemented.

[eldavojohn]

I looked into the linked article about the deprecation of unload. Is the reason to leave it as the default event to avoid the breaking change? If unload is going to be deprecated, it might be a good idea to change the default event, at least in the SDK 2.0 which is being currently planned and implemented.

Right now, we're blocked from using bfcache on our site because of this unload event. I agree that some alternative should be found for folks to use as a clean up event but I also recognize that that is kind of a rug-pull and I wanted this PR to make as little impact as possible. There's also not a "clear winner" replacement for this event but we stand to benefit from bfcache enough. Once we test out binding events on my end I would be happy to raise a second PR with a suggestion and healthy debate. I will check the linting step in a couple hours.

Thanks for looking into this!

[t2t2]

Separate question: Is a shutdown needed on exporter anyway as it shouldn't have state - the closest required functionality would be in batcher where visibilitychange and pagehide events are already used:

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/platform/browser/export/BatchSpanProcessor.ts

Line 43 in f4b681d

this._pageHideListener = () => {

[MSNev]

Separate question: Is a shutdown needed on exporter anyway as it shouldn't have state - the closest required functionality would be in batcher where visibilitychange and pagehide events are already used:

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/platform/browser/export/BatchSpanProcessor.ts

Line 43 in f4b681d

this._pageHideListener = () => {

I agree with above, we should just remove (or let it die when chrome stops supporting unload) shutting down the exporter based on some "automatic" event as it doesn't really provide any functionality. And if there is an exporter that wants / needs to do something special during "unload" operations then I'd let that implementation add the required code.

Why? If we trigger a "Shutdown" in reaction to the wrong event and the environment (page) either doesn't "shutdown" (beforeunload gets cancelled by the user) or it gets rehydrated then the exporter (in theory) thinks it's dead while its still being used.

[eldavojohn]

And if there is an exporter that wants / needs to do something special during "unload" operations then I'd let that implementation add the required code.

I'd really like this approach and this does solve my immediate needs. How can I help make this happen?

[eldavojohn]

I have raised an alternative solution here: #4438 where the shutdown event listening is removed altogether and left for the consuming code to implement if desired.