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

chore: remove unload event from OTLPExporterBrowserBase #4438

Merged

Conversation

eldavojohn
Copy link
Contributor

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

@eldavojohn eldavojohn requested a review from a team January 24, 2024 13:31
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

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.

@eldavojohn eldavojohn changed the title chore: remove shutdown event from OTLPExporterBrowserBase chore: remove unload event from OTLPExporterBrowserBase Jan 25, 2024
@MSNev
Copy link
Contributor

MSNev commented Jan 26, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

MSNev commented Jan 29, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

dyladan commented Feb 14, 2024

@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
Copy link
Contributor Author

eldavojohn commented Feb 14, 2024

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

dyladan commented Feb 14, 2024

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
Copy link
Contributor Author

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!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented Feb 14, 2024

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

Copy link

codecov bot commented Feb 14, 2024

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

@pichlermarc pichlermarc merged commit 356ef8a into open-telemetry:main Feb 15, 2024
20 checks passed
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…ry#4438)

* Add the ability for the implementation to provide the binding event for shutdown.

* fix: whoops totally missed the linting formatting fix step

* Remove any concept of a shutdown event

* add change log and obey interface

* no longer need globalthis

* modify the correct changelog and identify this as a breaking change under the scenario in comments

* markdown lint

* Update experimental/CHANGELOG.md

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants