Track cache tag queries separately in performance tests

Looks good to me. Tests also went green on my local and the changes will help me in reducing the expected results of #3421164: Log every individual query in performance tests

Still RTBC for all I care as you can fix on commit, but minor copy-paste mistake in a comment.

A few questions + nits 😇 But … I'm definitely intrigued! 🤩

I applied the nits, the only two things still left for discussion are:

• Do we keep using microtime()? -> Broader than this issue, should perhaps discuss in a follow-up
• Should we switch to enum? -> Fine by me, but how do we use match() then?

During one of many tests locally, StandardPerformanceTest::testAnonymous() failed on the checksum count:
150 is greater or equal to 143 and is smaller or equal to 146

This happened on only one out of 10ish test runs

Broader than this issue, should perhaps discuss in a follow-up

+1 for follow-up to maintain consistency. Tagged for that.

Should we switch to enum? -> Fine by me, but how do we use match() then?

Posted suggestion (https://git.drupalcode.org/project/drupal/-/merge_requests/6643/diffs#no...), proof that it works: https://3v4l.org/VsQ3n

Edit: Leaving below comment up, but prefacing with the fact that, after reading the docs on php.net, it seems common practice to seemingly misuse the match() statement for the sake of brevity. So I withdraw my insistence on using switch() even though semantically it seems like the right choice.

Having said that, I am a bit appalled at how lenient the match() statement is in terms of what you can do inside of it. Things like match(true) followed by a bunch of unrelated conditions and actual function calls returning any random value that is not even used afterwards feels really, really dirty. We're sacrificing readability and purpose for the sake of being able to write something in a few lines fewer.

While a match() definitely looks nicer and is more concise, I'm worried about the fact that we're not using it the way it was intended. We're executing an increment in a place where you're supposed to return a value and I'm not sure if php will keep allowing that for all eternity.

Would you agree with a switch() instead?

        switch($operation) {
          case CacheTagMetrics::getCurrentChecksum:
            $cache_tag_checksum_count++;
            break;

          case CacheTagMetrics::isValid:
            $cache_tag_is_valid_count++;
            break;

          case CacheTagMetrics::invalidateTags:
            $cache_tag_invalidation_count++;
            break;

          default:
            throw new \LogicException('Uncaught cache tag operation found.');
        };

The exception also takes care of your concern that we'd forget to count a newly monitored operation. I mean, it's definitely more verbose than the match() statement, but semantically it seems more correct?

Yeah, let's go with match(). I do agree it's far more readable.

I withdrew my objection in the edit above. I just feel like this is a regression to oldschool php where things have magic meanings. How would you teach the difference now between switch() and match() if the latter can be duckpunched into behaving like the former :)

#12: let match() grow on you 😊 It had to grow on me too. The #1 benefit of match() is not brevity, but the language-level help to ensure all cases are covered — see UnhandledMatchError.

#14: read https://stitcher.io/blog/php-8-match-or-switch — I'd argue the opposite is true: it's switch() that contains more magic than match() 😅

Keeping at Needs work for implementing match() since both @catch and @kristiaanvandeneynde are on board now 😊

Switched to enum/match and also made sure that we use the name property to get a string we can send to OpenTelemetry as I've read enums do not support __toString().

Furthermore made sure we actually send the cache tag stuff to OT (in our case Grafana) too and found something interesting. We are getting a bunch of calls where no tags were provided. See attached screenshot below. Only the one at the bottom actually had any tags, many of the resulting spans had none.

Come to think of it, these empty checks would explain the high number of cache tag operations we're seeing. Implementing code knows it's safe to call these operations with empty arrays and therefore does not bother checking how many tags there are before sending the (empty) list to the respective methods.

If we were to run an empty check before we call any operation, the number would go down drastically.

Regarding declare(strict_types=1); vs declare(strict_types = 1); I found >100 and ~90 use cases of them respectively. So unless we want to standardize on those I'd just leave it as is. PhpStorm seemed to manually add the spaces on my end.

🚀 This will be very helpful to improve Drupal's use of cache tags 😊 Thanks for making this happen!

Oh and I think we already need a follow-up for the first finding, by @kristiaanvandeneynde in #16: we can optimize \Drupal\Core\Cache\CacheTagsChecksumTrait::isValid() with a trivial early return:

if (empty($tags)) {
  return TRUE;
}

Committed and pushed d66403c93d to 11.x and d552da855a to 10.3.x. Thanks!