Problem/Motivation

It would be really useful to be able to expose data from Commerce, such as:

  • Orders and the state they are in.
  • Revenue from orders.
  • The number of products.

Proposed resolution

Add a submodule that exposes these metrics, and anything else in the future in the realm of Drupal Commerce.

Since calculating current metrics can be slow (several seconds of database queries on my test site with nearly 1 million order records), we need to break this into two phases:

1. A hook_rebuild implementation that creates the metrics, so we catch any changes in data applied by update hooks.
2. Event subscribers that increment and decrement the metrics.

The following metrics are exposed:

commerce_orders_revenue_total, with labels for order state, currency, administrative area, and site name.
commerce_orders_total, with labels for state, administrative area, and site name.
commerce_products_total, counting product entities, with labels for type and site name.

commerce_orders_revenue_total{state="completed",currency="USD",administrative_area="AA",site_name="Example.com"} 123
commerce_orders_total{state="canceled",administrative_area="MA",site_name="Example.com"} 74
commerce_products_total{type="egift_card",site_name="Example.com"} 23

Remaining tasks

  1. Do we want to push the RebuildableInterface up to the base module? The idea of rebuilding metrics after a deployment seems likely to be useful.
  2. We would like to be able to add labels for site specific data. I'm thinking that could be a separate issue that fires an event when building a metric?

User interface changes

None (so far!)

API changes

None.

Data model changes

None.

Issue fork prometheus_metrics-3556711

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

deviantintegral created an issue. See original summary.

deviantintegral opened merge request !5

deviantintegral’s picture

deviantintegral commented
Issue summary: View changes
deviantintegral’s picture

deviantintegral commented
Issue summary: View changes
deviantintegral’s picture

deviantintegral commented
Assigned: deviantintegral » Unassigned
Status: Active » Needs review

I've got unit and kernel tests in place, and they identified some deprecated code. They also identified that any exception rebuilding metrics would cause them all to stop, so I changed that to have one catch per method call.

While I have a few questions, I think this is ready for review!

deranga’s picture

deranga commented

Thanks for opening this feature request and the work you've put into it. I'll try to put aside some time this weekend to review and get back to you.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Needs work

The code looks great!

I’ve added a few comments regarding Drupal core compatibility.

While this is outside the scope of the current issue, the simplest long-term solution might be to create a new module release that drops support for Drupal core versions earlier than 10.3. This would allow the removal of several core-version checks currently present in the parent module.

The module maintainers can decide how to proceed — either by updating this merge request to ensure compatibility with older Drupal versions, or by opening a separate MR aimed at stabilizing the codebase for Drupal 10.3+ and merging this one afterward.

Another option would be to address the compatibility changes directly in this MR, though that would likely expand the scope more than necessary.

deranga’s picture

deranga commented

I've made a few changes.

  1. Added commerce product to required modules for the sub module
  2. Moved the rebuild interface up to the parent module as I like the concept. I do question tying it to the cache rebuild though as the metrics shouldn't be cleared from a cache rebuild, they should be cleared via the admin form. Have you experienced different?
  3. I've also incremented the core requirement to be 10.3+ and removed the version checking in the parent module
  4. I haven't looked into the other clean up items mentioned.
deviantintegral’s picture

deviantintegral commented

All your changes look good to me. I pushed up edits based on the review.

metrics shouldn't be cleared from a cache rebuild, they should be cleared via the admin form

We haven't deployed this yet, but I'm imagining a future scenario where a developer makes changes to orders directly in the database in an update hook. This could be in a contrib module, but more likely I'd guess custom code. I don't know if they're realize they need to rebuild metrics at the same time.

The other scenario I can think of is on managed hosts like Acquia and Pantheon. They often restart containers after a deployment, and don't support persistence in Redis or Memcache. In that scenario, the data is gone, but a cache rebuild will get it back up.

On the site I'm testing with, it has nearly 1 million orders, and takes around 2.5 seconds to rebuild all the stats. I'm not sure if we need to consider the case with 10 million orders or more.

deviantintegral’s picture

deviantintegral commented
Status: Needs work » Needs review

I enabled gitlab testing and fixed some tests. I think this is ready for another look!

deranga’s picture

deranga commented

I've made a few changes, and cleaned up a few bits.

I've added an item to the configuration form that allows the admin/ user to optionally wipe and rebuild metrics. This dispatches an event that can be optionally subscribed to. I've implemented an example of this in the parent module, would it be possible for you to do the same for the sub module and remove it from happening on cache rebuild?

I've done it this way as I'm not keen on the idea of it being automatically triggered on cache rebuild.

If you feel we need it to happen on cache rebuild, then my suggestion is that we add an option for this behaviour to the parent module config form, and we dispatch the same event from the parent module when the cache is rebuilt if that option has been enabled.

I've cleaned up for PHPCS and PHPStan and added some pre-commit hooks to try and keep it clean; let me know if this causes issues and I'll separate that out from the repo.

deranga’s picture

deranga commented

Made several more changes.

  • I've implemented the config item mentioned above.
  • Adjusted the sub module to handle based on subscribing to event.
  • Updated more of the parent module to use modern hook style.
  • Incremented core requirement to 11 to support modern hooks.

Let me know if any questions or issues.

Think this is now ready for a final review before merging. Would appreciate someone else taking a look if feasible.

deviantintegral’s picture

deviantintegral commented

Your changes all good to me. I've pushed up one text change, because Pantheon notably doesn't commit to long-term persistence of data in Redis. They don't back it to disk at all, and only support it as a cache. Look OK?

I've cleaned up for PHPCS and PHPStan and added some pre-commit hooks

I'm developing with ddev locally, so these are fine in that they just don't do anything if I don't install the hooks. However, I did notice some changes that don't follow Drupal standards, such as the array indentation in `\Drupal\Tests\prometheus_metrics_commerce\Kernel\OrderRevenueTest::createOrder`. Honestly, it's fine by me to finish the issue and clean up formatting in a followup.

  • deranga committed 16a6a102 on 1.0.x 
    feat: #3556711 Add a submodule exposing metrics from Drupal Commerce....
deranga’s picture

deranga commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

deranga’s picture

deranga commented
Status: Fixed » Closed (fixed)