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

MWPW-149470 [MILO][MEP] Remove sendTargetResponseAnalytics function #3599

Closed
wants to merge 2 commits into from

Conversation

vgoodric
Copy link
Contributor

  • Remove all of the AA tracking for how long Target response takes
  • Add in lana log tracking for when Target took longer than the page's Target timeout setting

Resolves: MWPW-149470

Test URLs:

@vgoodric vgoodric requested review from chrischrischris, swamu and a team January 30, 2025 22:30
Copy link
Contributor

aem-code-sync bot commented Jan 30, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Contributor

aem-code-sync bot commented Jan 30, 2025

Page Scores Audits Google
📱 /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.52%. Comparing base (cf4c08b) to head (fee6fd9).
Report is 48 commits behind head on stage.

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3599      +/-   ##
==========================================
- Coverage   96.53%   96.52%   -0.01%     
==========================================
  Files         274      274              
  Lines       61849    61815      -34     
==========================================
- Hits        59705    59670      -35     
- Misses       2144     2145       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

) {
let targetManifests = [];
let targetPropositions = [];
if (config?.mep?.enablePersV2 && targetInteractionPromise) {
const { targetInteractionData, respTime, respStartTime } = await targetInteractionPromise;
sendTargetResponseAnalytics(false, respStartTime, calculatedTimeout);
const { targetInteractionData, respTime } = await targetInteractionPromise;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the respStartTime from the return value of targetInteractionPromise in utils?
Since it is used at only this page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, maybe I'm misunderstanding the comment. I am using it on line 1197. So 5 lines later

Copy link
Contributor

@swamu swamu Feb 4, 2025

Choose a reason for hiding this comment

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

@vgoodric, the targetInteractionPromise function is defined in utils.js and returns the respStartTime. Since this method is only invoked here and the respStartTime is no longer needed, we can remove it from the return value in utils.js as well.

So the code in utils.js changes from:

targetInteractionPromise = (async () => {
     const { loadAnalyticsAndInteractionData } = await import('../martech/helpers.js');
     const now = performance.now();
     performance.mark('interaction-start');
     const data = await loadAnalyticsAndInteractionData(
       { locale, env: getEnv({})?.name, calculatedTimeout, hybridPersEnabled },
     );
     performance.mark('interaction-end');
     performance.measure('total-time', 'interaction-start', 'interaction-end');
     const respTime = performance.getEntriesByName('total-time')[0];

     return { targetInteractionData: data, respTime, respStartTime: now };
   })();

to

targetInteractionPromise = (async () => {
     const { loadAnalyticsAndInteractionData } = await import('../martech/helpers.js');
     performance.mark('interaction-start');
     const data = await loadAnalyticsAndInteractionData(
       { locale, env: getEnv({})?.name, calculatedTimeout, hybridPersEnabled },
     );
     performance.mark('interaction-end');
     performance.measure('total-time', 'interaction-start', 'interaction-end');
     const respTime = performance.getEntriesByName('total-time')[0];

     return { targetInteractionData: data, respTime };
   })();

@vgoodric vgoodric marked this pull request as draft February 11, 2025 16:56
@vgoodric vgoodric closed this Feb 11, 2025
@vgoodric
Copy link
Contributor Author

On hold so we can use this to measure pznv2

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.

3 participants