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

added a custom metric to store the number of localstorage and session… #124

Conversation

shaoormunir
Copy link

This pull request adds a new custom metric that logs how many times localStorage and sessionStorage was accessed in each response body in $WPT_BODIES. It returns a dictionary with the keys containing URLs and the values containing another dictionary containing numLocalStorage and numSessionStorage, with the former representing the number of times localStorage API was accessed while the later representing how many times sessionStorage API was accessed.

The code was tested on webpagetest.org.


Test websites:

@tunetheweb
Copy link
Member

I question the value in the counts.

For a start this file has the following:

e.localStorage&&e.localStorage.getItem(n)

Which logs two values, when it's only one.

Not to mention this logs two items:

localStorage.setItem(n);
localStorage.getItem(n);

Also if you have a wrapper function the counts are inaccurate:

function getValue(key) {
   return e.localStorage.getItem(key)
}
getValue('barry1');
getValue('barry2');
getValue('barry3');
getValue('barry4');
getValue('barry5');

Will log as one call, and not 5.

@shaoormunir
Copy link
Author

shaoormunir commented Jun 7, 2024

@tunetheweb for the first case, I have updated to only check if an item is added, removed or retrieved. This removes the extraneous counts which do not account for actual access to the API. For the second issue where there is a wrapper function, I do not see a good universal way (relying just on string matching) which would work for such cases. Do you have any suggestions around that?

Copy link

github-actions bot commented Jun 7, 2024

Custom metrics for https://almanac.httparchive.org/en/2022/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240607_8R_9

@tunetheweb
Copy link
Member

My suggestion was just to register use of them rather than counts of usage (since I think it's not very accurate).

@shaoormunir
Copy link
Author

I think for comparison purposes it is still useful. Logging that script uses the API 30 times vs one that uses it only 2-3 times, even if it is slightly under/over counted is still beneficial for analysis.

@tunetheweb
Copy link
Member

@rviscomi any opinions here? I'm fine to merge it, but worry it'll lead to inaccurate assumptions as to what it's actually measuring. Then again, maybe that could be said of any data we collect 😁

@shaoormunir in the meantime can you fix the linting errors.

@rviscomi
Copy link
Member

rviscomi commented Jun 7, 2024

Where usage counters aren't enough, I'd prefer to hold off on API usage detection like this until we get #54 working again for a more robust approach. I worry about the false positives from unused code and false negatives from function calls like in @tunetheweb's example, which would make it very hard to reason about the results.

@tunetheweb
Copy link
Member

Closing this following @rviscomi 's comment in #124 (comment)

@tunetheweb tunetheweb closed this Jun 19, 2024
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