perf milestone and measurement updates for minecraft #10309
Conversation
| @@ -38,20 +38,50 @@ namespace pxt { | |||
| let eventLogger: TelemetryQueue<string, Map<string>, Map<number>>; | |||
| let exceptionLogger: TelemetryQueue<any, string, Map<string>>; | |||
|
|
|||
| type EventListener<T = any> = (payload: T) => void; | |||
There was a problem hiding this comment.
What's the purpose of having a generic here if we're setting it to any?
There was a problem hiding this comment.
any is just a default type, effectively making the type optional. Some event sources might not want to pass a parameter. I do provide an explicit type in the instances below (via createEventSource).
| // performance measuring, added here because this is amongst the first (typescript) code ever executed | ||
| export namespace perf { | ||
| let enabled: boolean; | ||
|
|
||
| export const onMilestone = createEventSource<{ milestone: string, time: number, params?: Map<string> }>(); |
There was a problem hiding this comment.
Is there a reason we wouldn't want a type for event sources? I know these two objects have differences, but is there a foundation we would want to have for event sources?
There was a problem hiding this comment.
They do have a type, it's the type returned by createEventSource. I think I don't understand the question. Can you clarify?
| } | ||
| report += `\n` | ||
| } | ||
| durations[msg] = duration; | ||
| } | ||
| console.log(report) | ||
| enabled = false; // stop collecting milestones and measurements after report |
There was a problem hiding this comment.
In doing a quick skim, it looks like enabled is otherwise only getting set at the initialization of the perf loading. If this is the case, will it be possible to grab more reports after the webapp first loading?
There was a problem hiding this comment.
I made this change because I believe it is undesirable to continue collecting perf measurements after the perf report is generated. These measurements accumulate in memory over time and never flush, which is bad if left unbounded (which it was). These metrics are most useful for gauging startup performance, so I think we don't want to continue collecting them once the editor is fully loaded. Let me know if I'm mistaken!
There was a problem hiding this comment.
If I understand correctly, these are pretty generic methods. I worry someone might want to use them down the line (to measure issues with perf when hitting the play button, for example) and get confused as to why they're not working.
Maybe we could have a flag to control this behavior?
There was a problem hiding this comment.
@thsparks I agree it could be useful beyond startup. Supporting this would require coming up with a flushing strategy beyond what exists today. That's out of scope of this change.
| pxt.analytics.trackPerformanceReport(); | ||
| } | ||
| pxt.perf.recordMilestone(Milestones.EditorContentLoaded); | ||
| if (!this.autoRunOnStart()) { |
There was a problem hiding this comment.
Why is this no longer nested?
There was a problem hiding this comment.
I wanted to send this milestone to minecraft regardless of whether the project being loaded is a tutorial.
| export let startTimeMs: number; | ||
| export let stats: { | ||
| // name, start, duration | ||
| durations: [string, number, number][], | ||
| durations: [string, number, number, Map<string>?][], | ||
| // name, event |
There was a problem hiding this comment.
| // name, event | |
| // name, event, params |
| } | ||
| report += `\n` | ||
| } | ||
| durations[msg] = duration; | ||
| } | ||
| console.log(report) | ||
| enabled = false; // stop collecting milestones and measurements after report |
There was a problem hiding this comment.
If I understand correctly, these are pretty generic methods. I worry someone might want to use them down the line (to measure issues with perf when hitting the play button, for example) and get confused as to why they're not working.
Maybe we could have a flag to control this behavior?
Co-authored-by: Thomas Sparks <69657545+thsparks@users.noreply.github.com>
Adds three new extension points for sharing telemetry with the target:
In addition: