chore: remove unload event from OTLPExporterBrowserBase

[eldavojohn]

As discussed in #4326 we could remove any concept of a shutdown event and allow consuming code to handle that explicitly.

Which problem is this PR solving?

The 'unload' event is stopping any site using OTEL in JavaScript from taking advantage of back/forward cache. And also Google is planning to deprecate unload.

Fixes # (issue)

Short description of the changes

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

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

[pichlermarc]

Hi there, thanks for opening the PR. 🙂
Looks like a changelog entry is missing, you'll need to add an entry to ./CHANGELOG.md to contiune.

[MSNev]

I don't think we should just be removing the "unload" event. I understanding the ask but there is a bunch of nuances that are bing missed here.

• This is (currently) specific to chromium browsers
• Removing will cause any extension classes to have to either "listen" to the unload events ("unload", "beforeunload", "pagehide", "visibility") to ensure that any "batched" events are flushed.
• This class is not listening to all possible "unload" events and the "unload" event is not always called either (we should at the very least have "pagehide" (for IOS) and "visibilitychange" (all other modern). While "unload" and "beforeunload" are required for "older" runtimes that don't support the other 2.

Rather than "remove", this "should" be configuration driven and we should include the others.

Happy to consider that the "default" is false (to not include by default), or to have code that "checks" if the runtime is Chromium based with version > the say 100 (or something -- not sure when it occurred).

And while it's being "deprecated" / "removed" the code should "handle" (identify) when the event was not able to be hooked (because the addEventListener returns false).

Simply, just Removing this should also be considered a breaking change because of the downstream functionality change.

[eldavojohn]

I don't think we should just be removing the "unload" event. I understanding the ask but there is a bunch of nuances that are bing missed here.

I guess I'm confused, what was being proposed in your comment here? #4326 (comment)

[MSNev]

I don't think we should just be removing the "unload" event. I understanding the ask but there is a bunch of nuances that are bing missed here.

I guess I'm confused, what was being proposed in your comment here? #4326 (comment)

This PR just deleting the code is not how this should be working. It should be behind a flag about whether to add / remove the unload events. And preferablly, that flag should be an array of the unload events (as listed above).

[eldavojohn]

This PR just deleting the code is not how this should be working. It should be behind a flag about whether to add / remove the unload events. And preferablly, that flag should be an array of the unload events (as listed above).

Okay thanks for the further explanation, do you mind describing or pointing to an existing example of how flag is used in this repository? Greping for flag I don't really see anything that is obvious to me how it would be used ... are you just talking about adding another property on experimental/packages/otlp-exporter-base/src/OTLPExporterBase.ts?

[eldavojohn]

And while it's being "deprecated" / "removed" the code should "handle" (identify) when the event was not able to be hooked (because the addEventListener returns false).

I'm having a very difficult time trying to figure out what you're asking for ... can you tell me more about the addEventListener returning false? In the MDN docs it says it just returns undefined and the lib dom types declaration says it's void return type:

declare function addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;

I've updated the original PR to take an array as an optional argument -- otherwise it should perform just as it did before.

[dyladan]

@eldavojohn we discussed this in the SIG today and concluded that we should be fine just removing the functionality, but we want to list it as a breaking change. The possible break is this:

1. A user extends the exporter and overrides the shutdown function, and does something which is usually called by the unload listener
2. We remove the unload event listener
3. That user's overridden shutdown function no longer gets called

Please let me know if you have questions. Can probably close #4326

[eldavojohn]

Hey @dyladan thanks ... um, I keep reading "removing the functionality" and I'm just absolutely confused. In the PR I closed, I allowed the event to be defined by the consuming code. That seemed to be wrong. So in this PR I remove the functionality. Which also seems to be wrong. I understand the breaking change from "removing the functionality" but I am absolutely struggling with what that means if it doesn't mean deleting the event listener that is listening for the unload event. I'm sorry becuase these seem like very atomic concepts and I'm unable to grasp them and I thank you for your time and patience!

[dyladan]

I think you mixed up the PRs? This one doesn't allow any property to be set. This PR is the correct solution based on what we discussed in the meeting today.

[eldavojohn]

I think you mixed up the PRs? This one doesn't allow any property to be set. This PR is the correct solution based on what we discussed in the meeting today.

Sorry I definitely did, I thought this PR was horrendously wrong as explained by @MSNev . Please let me know if there's anything else I can do or any changes to this code. Thanks again!

[dyladan]

I think you mixed up the PRs? This one doesn't allow any property to be set. This PR is the correct solution based on what we discussed in the meeting today.

Sorry I definitely did, I thought this PR was horrendously wrong as explained by @MSNev . Please let me know if there's anything else I can do or any changes to this code. Thanks again!

We discussed this with @MSNev today and he is now convinced this is the correct approach

[codecov]

Codecov Report

Merging #4438 (f04564f) into main (5bc8ced) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4438      +/-   ##
==========================================
+ Coverage   92.41%   92.43%   +0.01%     
==========================================
  Files         330      330              
  Lines        9525     9525              
  Branches     2033     2033              
==========================================
+ Hits         8803     8804       +1     
+ Misses        722      721       -1     

see 1 file with indirect coverage changes