Write timings to file on shutdown & add API for writing timings file

[remminiscent]

if you have Timings enabled, as well as the option set to true in pocketmine.yml, when the server CORRECTLY shutsdown, itll provide 1 last timings report.

Related issues & PRs

• Fixes Timings auto paste #6158
• Fixes Allow pasting timings using an API method #5644

Changes

API changes

maybe TimingsHandler::uploadReport & TimingsHandler::createReportFile

Follow-up

possibly make it periodically send timings, given interval will also be in pocketmine.yml

Tests

https://streamable.com/38giks

[dktapps]

The createReportFile() and uploadReport() functions should handle collecting the timings as well as generating the files, so that the API isn't inconvenient for plugins. You'll want to return Promise to allow the caller to do stuff before & after.

[ShockedPlot7560]

In more for me

[dktapps]

Per internal discussion, I think it would be better to:

• Always create a timings file locally on shutdown (if timings was enabled, ofc)
• Get rid of the auto paste - users will probably leave it enabled by mistake and accidentally upload timings reports they weren't aware of
• Change the file naming of timings files to be more like crashdumps

I think if we generate a report file, the user can then do what they want with the file (including uploading it manually), without running the risk of spamming the timings server or accidentally uploading stuff they didn't intend to.

[remminiscent]

did what you requested, but i removed the ability to give custom names to the report file from external api usage, is that fine with you? or bring it back.

could be optional param set to null, if custom string is provided, itll use that. obviously with the index check for it too

[dktapps]

could be optional param set to null, if custom string is provided, itll use that. obviously with the index check for it too

I'd say allow it to be specified, but if it already exists, just bail out.

[remminiscent]

could be optional param set to null, if custom string is provided, itll use that. obviously with the index check for it too

I'd say allow it to be specified, but if it already exists, just bail out.

wdym "just bail out"? also for the file path, i dont think we make it mandatory.

[remminiscent]

done 👍

[dktapps]

The file report side looks mostly fine.

I still don't like the way the paste/upload stuff is handled though. It's inconvenient for plugins to need a CommandSender, and they also don't have any way to collect the timings URL with the current design. We also probably don't want to broadcast command audit messages when a plugin uses the upload API, for the same reason we don't want it broadcasting audit messages when generating a report on shutdown - it's not really the sender doing the action.

Since we don't need the upload API for implementing shutdown timings dumping, I'd be OK with reverting that part of the change and only doing the file report side for now. I think the upload stuff might require some thinking about how to deal with upload errors. The Promise API doesn't have any way to pass error information to the reject callback currently.

[remminiscent]

The file report side looks mostly fine.

I still don't like the way the paste/upload stuff is handled though. It's inconvenient for plugins to need a CommandSender, and they also don't have any way to collect the timings URL with the current design. We also probably don't want to broadcast command audit messages when a plugin uses the upload API, for the same reason we don't want it broadcasting audit messages when generating a report on shutdown - it's not really the sender doing the action.

Since we don't need the upload API for implementing shutdown timings dumping, I'd be OK with reverting that part of the change and only doing the file report side for now. I think the upload stuff might require some thinking about how to deal with upload errors. The Promise API doesn't have any way to pass error information to the reject callback currently.

i agree with the CommandSender param, ill see if i can figure a workaround, till then i did your requested changes

[dktapps]

Sorry for giving you so much of a runaround with this. Obviously a lot of these issues aren't really your fault because you're just moving around existing code. But in doing so it's highlighting new problems.

[remminiscent]

Sorry for giving you so much of a runaround with this. Obviously a lot of these issues aren't really your fault because you're just moving around existing code. But in doing so it's highlighting new problems.

it's absolutely fine, i do not mind at all. I guess our only issues are that in the upload function, we need to figure a way to not require a CommandSender, and that Promise does not provide a way to send an error "message" for it's rejection. Perhaps someone else can come up with a better way later. but for now ill finish up the shutdown timings report.

[remminiscent]

for future reference, and maybe future PRs by me, why do you dislike Server::getInstance()?

[dktapps]

for future reference, and maybe future PRs by me, why do you dislike Server::getInstance()?

Because the dependencies aren't outwardly obvious and it's a pain for unit testing. Singletons in general are problematic in that regard.

[dktapps]

Almost done I think

[dktapps]

Just one last formatting fix and this is good to go I think