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

Add custom metrics for the WordPress Interactivity API #113

Conversation

adamsilverstein
Copy link
Contributor

@adamsilverstein adamsilverstein commented Apr 3, 2024

Collect data about usage of the WordPress interactivity API, which recently shipped in WordPress 6.5 (see https://make.wordpress.org/core/2024/03/04/interactivity-api-dev-note/)

Adds two new datapoints to the cms/wordpress custom metric:

  • uses_interactivity_api - does the page uses the Interactivity API?
  • interactivity_api_region_count - an array with the region count by namespace.

Test websites:

Current results:

Copy link
Contributor

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@adamsilverstein Leaving a bit of feedback. I think some of this needs to be refined a bit more in which data we want to include. Also we should differentiate between Interactivity API "regions" and any elements that use the Interactivity API.

dist/cms.js Outdated Show resolved Hide resolved
dist/cms.js Outdated Show resolved Hide resolved
@adamsilverstein
Copy link
Contributor Author

@adamsilverstein Leaving a bit of feedback. I think some of this needs to be refined a bit more in which data we want to include. Also we should differentiate between Interactivity API "regions" and any elements that use the Interactivity API.

Thanks for the feedback, I like the idea of summarizing just to the counts by type, I'm going to build out some test pages to clarify the best way to do this.

@adamsilverstein
Copy link
Contributor Author

adamsilverstein commented Apr 4, 2024

Updates:

  • New naming for the returned fields to better match existing custom metrics (like embed)
  • Count the types (namespaces) and return that in the interactivity_api_region_count metric

dist/cms.js Outdated Show resolved Hide resolved
dist/cms.js Outdated Show resolved Hide resolved
* namespace not type
* make top level more generic `total_regions_by_namespace`
dist/cms.js Outdated Show resolved Hide resolved
@pmeenan
Copy link
Member

pmeenan commented Apr 8, 2024

FYI, the April crawl kicks off Wednesday. Not sure if you want this metric in by then but, if so, it'll need to land today so I can update the agents before it kicks off.

@tunetheweb
Copy link
Member

@adamsilverstein I updated the first comment to the correct heading needed to trigger the test ("Test websites:" rather than "Test page"). So if you push a commit now it should kick that off for your test site.

dist/cms.js Outdated Show resolved Hide resolved
@adamsilverstein
Copy link
Contributor Author

@adamsilverstein I updated the first comment to the correct heading needed to trigger the test ("Test websites:" rather than "Test page"). So if you push a commit now it should kick that off for your test site.

@tunetheweb - Ah, thanks! I probably changed that from the template because I was only linking to one website (and I didn't realize this triggered the test).

I noticed when the test action ran, the results don't include the full region counts, ie:
image
total_regions_by_namespace shows as empty.

Is that because the output doesn't show the full data, or something off with the code? The code did work correctly when I ran in the console. I'll try running in WPT through the UI.

@tunetheweb
Copy link
Member

Is that because the output doesn't show the full data, or something off with the code? The code did work correctly when I ran in the console. I'll try running in WPT through the UI.

It's because we can't JSON.stringify an Array

The same thing happens in DevTools if you do this:

function getInteractivityAPIUsage() {
  // Look for data-wp-interactive regions.
  const interactiveRegions = document.querySelectorAll('[data-wp-interactive]');

  // Count the regions by namespace.
  const regionNamespaceCounts = []
  interactiveRegions.forEach( region => {
    // Extract the namespace from JSON or as a string
    let namespace = '';
    const regionAttribute = region.getAttribute('data-wp-interactive');
    try {
      const regionData = JSON.parse( regionAttribute );
      if ( regionData.namespace && 'string' === typeof regionData.namespace ) {
        namespace = regionData.namespace;
      }
    } catch ( e ) {
      namespace = regionAttribute;
    }
    if ( '' !== namespace ) {
      regionNamespaceCounts[ namespace ] = ( regionNamespaceCounts[ namespace ] || 0 ) + 1;
    }
  })

  console.log(regionNamespaceCounts, JSON.stringify(regionNamespaceCounts));

  return JSON.stringify({
    total_regions: interactiveRegions.length,
    total_regions_by_namespace: regionNamespaceCounts,
  });
}
getInteractivityAPIUsage();

Will suggest something.

dist/cms.js Outdated Show resolved Hide resolved
Comment on lines +182 to +185
return {
total_regions: interactiveRegions.length,
total_regions_by_namespace: regionNamespaceCounts,
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return {
total_regions: interactiveRegions.length,
total_regions_by_namespace: regionNamespaceCounts,
};
return JSON.stringify({
total_regions: interactiveRegions.length,
total_regions_by_namespace: regionNamespaceCounts,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

@tunetheweb Why is this necessary? I personally would find it odd to find stringified JSON within data that's already JSON. That's always been a bit mysterious to me with HTTP Archive data (also in payload column), why do we have to JSON-decode twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question. The github-action did return the expected data:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to what @adamsilverstein shared. There isn't actually an array in the data, it's only objects.

But even generally, I'm not sure what's the limitation of using JSON.stringify() on an array? 🤔

Copy link
Member

@tunetheweb tunetheweb Apr 9, 2024

Choose a reason for hiding this comment

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

Why is this necessary? I personally would find it odd to find stringified JSON within data that's already JSON.

Yes as mentioned below you only need to stringify the outermost return. Apologies for the confusion.

That's always been a bit mysterious to me with HTTP Archive data (also in payload column), why do we have to JSON-decode twice?

Ah that's legacy thing. Some of the older custom metrics are double escaped. Yeah I don't like that either. We shouldn't do that anymore for newer custom metrics. Dunno why we did to be honest - before my time.

+1 to what @adamsilverstein shared. There isn't actually an array in the data, it's only objects.

It was (incorrectly) originally set to be an Array as mentioned in my comment above.

But even generally, I'm not sure what's the limitation of using JSON.stringify() on an array? 🤔

As described below the "limitation" is when you set up an array, more on when using "named" array entries, which aren't actually array entries at all:

const myArray = []
myArray[1] = 'value'; // this is fine
myArray['key'] = 'value' // this is incorrect

That last line is incorrect and actually is shorthand for myArray.key = 'value';
You've basically set an attribute on the array and not added a value to the array.
When you convert the array to a string, it doesn't include attributes (it would if it was an object, but it's not—it's an array). It only exports array entries to the string and in this example myArray only has one entry not two (as can be seen with myArray.length).

Why does JavaScript allow you to set attributes on an Array when it's almost certainly only ever done in error? 🤷‍♂️
Ultimately everything in JavaScript is an object so you can always set attributes.

Why does JavaScript have a similar syntax for setting array values and array attributes? 🤷‍♂️🤷‍♂️
Just one of the many fun things with JavaScript!

Copy link
Contributor

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @adamsilverstein, this looks solid to me. Just one small point of follow up feedback.

dist/cms.js Outdated Show resolved Hide resolved
Copy link
Contributor

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Copy link

github-actions bot commented Apr 8, 2024

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

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240408_EV_T

Custom metrics for https://interactivity-api.instawp.xyz/test-post/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240408_MY_V
Changed custom metrics values:

{
    "_cms": {
        "wordpress": {
            "block_theme": true,
            "has_embed_block": false,
            "embed_block_count": {
                "total": 0,
                "total_by_type": []
            },
            "scripts": [
                {
                    "handle": "jquery-core",
                    "src": "https://interactivity-api.instawp.xyz/wp-includes/js/jquery/jquery.min.js?ver=3.7.1",
                    "in_footer": false,
                    "async": false,
                    "defer": false,
                    "intended_strategy": null,
                    "after_script_size": null,
                    "before_script_size": null,
                    "extra_script_size": null,
                    "translations_script_size": null
                },
                {
                    "handle": "jquery-migrate",
                    "src": "https://interactivity-api.instawp.xyz/wp-includes/js/jquery/jquery-migrate.min.js?ver=3.4.1",
                    "in_footer": false,
                    "async": false,
                    "defer": false,
                    "intended_strategy": null,
                    "after_script_size": null,
                    "before_script_size": null,
                    "extra_script_size": null,
                    "translations_script_size": null
                },
                {
                    "handle": "ideabox-toc-script",
                    "src": "https://interactivity-api.instawp.xyz/wp-content/plugins/table-of-contents/assets/js/frontend.js?ver=1.0.2",
                    "in_footer": false,
                    "async": false,
                    "defer": false,
                    "intended_strategy": null,
                    "after_script_size": null,
                    "before_script_size": null,
                    "extra_script_size": null,
                    "translations_script_size": null
                }
            ],
            "content_type": {
                "template": "unknown",
                "post_type": "",
                "taxonomy": ""
            },
            "uses_interactivity_api": true,
            "interactivity_api_usage": {
                "total_regions": 12,
                "total_regions_by_namespace": {
                    "core/navigation": 4,
                    "myPlugin": 1,
                    "core/search": 1,
                    "core/image": 2,
                    "core/query": 2,
                    "core/router": 2
                }
            }
        }
    }
}
Custom metrics for https://github.com/HTTPArchive/custom-metrics/assets/2676022/65913bd4-eb52-4154-9438-b462f702e80c

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240408_QS_W
Changed custom metrics values:

{
    "_cms": {
        "wordpress": {
            "block_theme": false,
            "has_embed_block": false,
            "embed_block_count": {
                "total": 0,
                "total_by_type": []
            },
            "scripts": [],
            "content_type": {
                "template": "unknown",
                "post_type": "",
                "taxonomy": ""
            },
            "uses_interactivity_api": false,
            "interactivity_api_usage": {
                "total_regions": 0,
                "total_regions_by_namespace": []
            }
        }
    }
}

@adamsilverstein
Copy link
Contributor Author

It's because we can't JSON.stringify an Array

@tunetheweb Ah, right. This has caused me problems before (and yet, I tried again).

return JSON.stringify(

I'm not clear on this bit: we don't stringify other returns, for example the embed block data returned from getWordPressEmbedBlockCounts above... why is JSON.stringify needed here? (the data structure seems very similar to the embed data)

@tunetheweb
Copy link
Member

Ah sorry you don't need to JSON.stringify each function separately. I was testing with just the getInteractivityAPIUsage function call, and not the whole thing.

You SHOULD however JSON.stringify at the end.

return JSON.stringify({
  wordpress
};

JavaScript objects are NOT the same as JSON. They look very similar, but not quick the same. A JavaScript object is a structured object which can include references. JSON is just a string in a particular format. Not even JavaScript object can be cleanly converted to a JSON string.

Which is what we saw earlier, and what we can repeat here with this smaller bit of code:

const myArray = [];
myArray['subArray1'] = [1,2,3];
myArray['subArray2'] = [4,5,6];
console.log(myArray);
console.log(JSON.stringify(myArray));
image

The first line prints named arrays, the second just an empty array since you can't convert named arrays to JSON (they are basically parameters on the array variable, rather than values in the array).

The length of that array is 0 btw:

image

Because you are NOT pushing values in that array, you are setting attributes on that array. And when you stringify an array, it only looks at the values, not the attributes.

This however does work:

const myArray = [];
myArray[0] = [1,2,3];
myArray[1] = [4,5,6];
console.log(myArray);
console.log(JSON.stringify(myArray));
image

Anyway, HTTP Archive stores these as Strings. In most cases it will convert these automatically which is why you don't NEED it here. However I advise to use JSON.stringify() at the end. Why? Because then when you test this in the console you'll also see when it's missing stuff you expected to in there rather than be confused why WebPageTest's magic string conversion (which isn't magic at all) did something different than you expected.

Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

As discussed I think we should JSON.stringify the final value so when testing this in the console you can more clearly see when it's not including stuff you thought it would. That way it will seem less like HTTP Archive magic and more like JavaScript weirdness 😁

However, lets merge this for now and worry about that after this run.

dist/cms.js Show resolved Hide resolved
@tunetheweb tunetheweb merged commit 6d6d36b into HTTPArchive:main Apr 9, 2024
2 checks passed
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.

None yet

5 participants