Retroactive placeholder for easy future reference, and quarterly roll-up within perf team.
Description
Description
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Invalid | None | T309013 EditAttemptStep Migration to (monoschema) MP | |||
| Resolved | phuedx | T309985 Migrate WikiEditor EditAttemptStep instrument to Metrics Platform | |||
| Open | None | T276378 EPIC: Release Metrics Platform v1 | |||
| Open | None | T299916 Validate the new stream configuration properties | |||
| Resolved | phuedx | T281762 Incorporate librarized Metrics Platform PHP client into EventLogging | |||
| Resolved | Krinkle | T309656 Performance review for new Metrics Plaform JS library |
Event Timeline
Comment Actions
The process for "major" reviews is documented on mediawiki.org. I'd say this wasn't a major change per-se. What I think is important is that the relevance and potential of performance impact was recognised very early on by @phuedx, who then reached out and asked for input accordingly. At that point, I could have asked to file a review task, which I choose not to.
For future reference, most of this review happened directly in code review, which I think was quite efficient and effective for this case:
I shared a couple of tips on how I measure certain things and what we generally look for.
The main thing we identified was the increase in transfer size for the EventLogging payload: T309313: Minimise bytes transferred in Metrics Platform client.
.. of which the bulk was addressed via:
... which I gave feedback on, and after a library release, Sam then pulled this the same WIP commit draft we had before, and emperically verified the impact. I signed off with a +1 for subsequent merging and train deployment.
As part of the extension review, @Reedy and myself also covered some general extension and library best practices (some of which are documented for future reference: docs, more docs)
[PHP] composer.json: Swap from classmap to PSR-4
Comment Actions
As part of our regular monitoring, we'll follow-up if any unexpected fallout appears down the line. I'm confident we've covered what we can reasonly cover as part of the performance review and the tools and information is known and available to proactively keep an eye on this.
Feel free to reach out with any questions or follow-up, either here or directly to myself or the team by email.
Thanks!
Comment Actions
Thanks for taking the time to write this up, @Krinkle.
Should we go through a similar process for the PHP integration and integration with EventLogging?
Comment Actions
@phuedx Sure! Feel free to CC myself and Aaron in code review. I antipate a much smaller risk area here though as I assume server-logged events are almost entirely invisible from the client perception (no bytes transferred, deferred code execution).
As reference, check out our guidance on Backend performance and runbook for Backend measurement. While expensive computations can come in unexpected places, I imagine the main take-away here will be to use DeferredUpdates. Beyond that the only concern would be infrastructure cost or memcached use, but that takes a lot to exhaust and is not something we really have general guidance on beyond using your judgement.