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-159555 Add support for lana-sample query param #3045

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Conversation

chrischrischris
Copy link
Contributor

The lana-sample query param will override any sample rate defined elsewhere. Setting it to "100" will force lana to send everything to splunk.

Resolves: MWPW-159555

Test URLs:

The `lana-sample` query param will override any sample rate defined elsewhere.  Setting it to "100" will force lana to send everything to splunk.
@chrischrischris chrischrischris requested a review from a team as a code owner October 15, 2024 19:37
Copy link
Contributor

aem-code-sync bot commented Oct 15, 2024

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 Oct 15, 2024

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 Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.34%. Comparing base (ec48f3c) to head (a18ae2d).
Report is 45 commits behind head on stage.

Additional details and impacted files
@@           Coverage Diff           @@
##            stage    #3045   +/-   ##
=======================================
  Coverage   96.34%   96.34%           
=======================================
  Files         243      243           
  Lines       55284    55288    +4     
=======================================
+ Hits        53261    53265    +4     
  Misses       2023     2023           

☔ 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.

@SilviuLCF
Copy link

Hi @chrischrischris can you provide some guidance on how testing should be performed for this change?

thank you

libs/utils/lana.js Outdated Show resolved Hide resolved
@overmyheadandbody
Copy link
Contributor

Seems the new unit test is failing - ❌ LANA > The lana-sample query param overrides existing sampleRate

Copy link
Contributor

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.

Copy link
Contributor

@thedoc31 thedoc31 left a comment

Choose a reason for hiding this comment

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

Assuming someone will always send in a valid integer between 0 and 100 is dangerous without any (apparent) input validation or error handling. What happens if I send in an invalid number or a value long enough to be a BigInt? Would lana know what to do if it got a NaN in this param?

@chrischrischris
Copy link
Contributor Author

chrischrischris commented Oct 18, 2024

@thedoc31 The parseInt handles the sanitization of the input. If the lana-sample param is invalid it will be NaN and the next line will ignore it:

const sampleRateParam = parseInt(new URL(window.location).searchParams.get('lana-sample'), 10);
const sampleRate = sampleRateParam || (o.errorType === 'i' ? o.implicitSampleRate : o.sampleRate);

It's fine if the param is negative or a gigantic number. Negative will behave like 0, anything over 100 behaves like 100.

I've added a test case for an invalid param value.

Copy link

@SilviuLCF SilviuLCF left a comment

Choose a reason for hiding this comment

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

Verified, ready to STAGE

@thedoc31 thedoc31 dismissed their stale review October 22, 2024 15:33

The parseInt and existing code provide sufficient validation and the extra test case will validate that. Closing objection

@milo-pr-merge milo-pr-merge bot merged commit 56ed952 into stage Oct 28, 2024
19 checks passed
@milo-pr-merge milo-pr-merge bot deleted the mwpw-159555 branch October 28, 2024 09:18
@milo-pr-merge milo-pr-merge bot mentioned this pull request Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants