Persisting segment events in MetaMetricsController store #16198
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
jpuri
requested review from
adonesky1,
brad-decker,
Gudahtt,
PeterYinusa and
seaona
|
Test failures look legit. |
… segment_event_storage
…extension into segment_event_storage
Builds ready [0f5067f]
Page Load Metrics (2552 ± 77 ms)
highlights:storybook
|
Builds ready [0a6e834]
Page Load Metrics (2606 ± 224 ms)
highlights:storybook
|
Builds ready [c0889b9]
Page Load Metrics (2089 ± 118 ms)
highlights:storybook
|
Builds ready [9bfdae5]
Page Load Metrics (2573 ± 190 ms)
highlights:storybook
|
…extension into segment_event_storage
brad-decker
suggested changes
Nov 3, 2022
|
|
||
| this.store = new ObservableStore({ | ||
| participateInMetaMetrics: null, | ||
| participateInMetaMetrics: true, |
There was a problem hiding this comment.
this needs to be null to start or we will track events before they opt in.
There was a problem hiding this comment.
ops my bad, thanks for catching this
… segment_event_storage
Builds ready [ef5ae4a]
Page Load Metrics (2419 ± 83 ms)
highlights:storybook
|
brad-decker
approved these changes
Nov 4, 2022
|
@jpuri from QA looks good! I'm re-running Firefox e2e tests, as the installation script failed. Hopefully this fixes it! |
Builds ready [1805f8c]
Page Load Metrics (2055 ± 94 ms)
highlights:storybook
|
digiwand
reviewed
Nov 7, 2022
| // and pass a callback to remove it from store once request is submitted to segment | ||
| // Saving segmentApiCalls in controller store in MV3 ensures that events are tracked | ||
| // even if service worker terminates before events are submiteed to segment. | ||
| _submitSegmentAPICall(eventType, payload, callback) { |
There was a problem hiding this comment.
nit: in regards to eventType, why use event over method? I'm thinking events mean something different for data analytics and JavaScript
It could be helpful to add a description for eventType here. e.g.
@param {string} eventType - Segment API call name
@see {@link https://segment.com/docs/connections/spec/}
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16173
PR address issue of segment events being lost in MV3 due to service worker restart. Events are now saved in MetaMetricsController store and resend to segment SDK till they are successfully submitted to segment.