[bug] The cursor_size logger is misleading#26
[bug] The cursor_size logger is misleading#26kchro3 wants to merge 6 commits intoAutomattic:mainfrom
Conversation
| } | ||
|
|
||
| StreamBuilder::getDependencyBag()->getLog() | ||
| ->histogramTick('cursor_size', $context, ($cursor_size / 1000.0)); |
There was a problem hiding this comment.
Aren't we losing visibility in what is going to be cached with this change?
There was a problem hiding this comment.
i think we'd actually get a clearer picture of what size ended up being persisted somewhere, as the comment above points out
There was a problem hiding this comment.
might be worth adding an actual INFO or DEBUG level log here too, so we can get some hard data rather than just time series info
| } elseif (is_string($obj)) { | ||
| $json = $obj; | ||
| } else { | ||
| throw new \InvalidArgumentException(sprintf("%s type is not supported", get_class($obj))); |
There was a problem hiding this comment.
we should document this in the method's phpdoc, since now it doesn't match the parent class
| $cursor_size = strlen($json); | ||
| } | ||
|
|
||
| StreamBuilder::getDependencyBag()->getLog() |
There was a problem hiding this comment.
| StreamBuilder::getDependencyBag()->getLog() | |
| // We are tracking two things simultaneously: a Base64-encoded cursor that goes into a URL | |
| // and a JSON-encoded cursor that persists in the cache. | |
| StreamBuilder::getDependencyBag()->getLog() |
Is it going to be a bad idea to add another tick to track just the thing that goes into a URL, and leave the existing tick to track what gets cached?
Co-authored-by: Alexey Kopytko <alexey@kopytko.com>
Co-authored-by: cyle gage <cyle@cylesoft.com>
| * @inheritDoc | ||
| */ | ||
| public function encode(Templatable $obj): string | ||
| public function encode($obj): string |
|
I'm going to remove myself on this one as you've already got two chunks of feedback. Ping if you need me back for something. |
5 participants
See: (issue #)
What and why? 🤔
In the StreamCursor, the serialization logic attempts to use the BinaryCodec, but if the size exceeds some threshold (1800 bytes), it instead uses the CachedCodec. The CachedCodec will return a cache key and write the payload to cache behind-the-scenes.
The stats that are published represent the size of the BinaryCodec payload but does not account for the case where the CachedCodec is used. There is also a slight discrepancy, where the CachedCodec writes the payload as a plain JSON without base-64 url encoding, so the published stats could be inflated up to 33%: https://www.php.net/manual/en/function.base64-encode.php
This implementation also modifies the encode function to accept a serialized JSON string so that we wouldn't need to call
->to_template()multiple times. We already call it two times when the CachedCodec is used, without this change, we would need to call ->to_template() three times, so this should be an improvement.It is also worth noting that the current API where encode takes in an Templatable is not respected because if the CachedCodec serialization type is
SERIALIZATION_TYPE_PHP_OBJECT, the input wouldn't be a Templatable, so changing it fromencode(Templatable $obj)->encode(mixed $obj)is more accurate anyways..Testing Steps ✍️
Provide adequate testing steps (including examples if necessary).
Don't assume someone knows how to test your changes; be thorough.
You're it! 👑
Tag a random reviewer by adding
@Automattic/stream-buildersto the reviewers section, but mention them here for visibility as well.