Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collect better stats for snapshot loading (SPO) #20481

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

vladsud
Copy link
Contributor

@vladsud vladsud commented Apr 4, 2024

This PR

  • Collects better telemetry about tree / blob structure of snapshots. In particular, we want to know.
    • number of blobs vs. number of blob leaf nodes in the tree. This difference will be influenced by attachment blobs (missing from the snapshot) and any blob deduping service does. That said, this PR stops short of deduping these cases.
    • Provide better stats on number of trees, include leaf trees. We have "leafTrees" but it was counting all trees, not just leaf nodes.
  • Added "fast" parsing case for empty trees
  • Added asserts in unit tests that validate our telemetry about slow parsing paths is correct.
  • Deleted second version of evalBlobsAndTrees function that was not exported from package and was not used.

@vladsud vladsud requested review from pragya91 and jatgarg April 4, 2024 23:32
@github-actions github-actions bot added area: driver Driver related issues area: odsp-driver base: main PRs targeted against main branch labels Apr 4, 2024
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Apr 5, 2024

@fluid-example/bundle-size-tests: +254 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 452.49 KB 452.49 KB No change
azureClient.js 545.05 KB 545.05 KB No change
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 255.4 KB 255.4 KB No change
fluidFramework.js 344.58 KB 344.58 KB No change
loader.js 129.36 KB 129.36 KB No change
map.js 41.35 KB 41.35 KB No change
matrix.js 143.61 KB 143.61 KB No change
odspClient.js 513.28 KB 513.36 KB +83 Bytes
odspDriver.js 97.29 KB 97.37 KB +83 Bytes
odspPrefetchSnapshot.js 41.86 KB 41.94 KB +88 Bytes
sharedString.js 161.38 KB 161.38 KB No change
sharedTree.js 334.69 KB 334.69 KB No change
Total Size 3.14 MB 3.14 MB +254 Bytes

Baseline commit: 7829364

Generated by 🚫 dangerJS against 501c9fb

for (const [_, tree] of Object.entries(snapshotTree.trees)) {
numTrees += 1;
numTrees += countTreesInSnapshotTree(tree);
function getTreeStatsCore(snapshotTree: ISnapshotTree, stats: ITreeStats): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: suggestion to place the function next to the getTreeStats above since they are related.

Or better yet, we can move these tree stat calculating functions to a different file or odspUtils

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, but let's do it in separate PR. I prefer to separate mechanical changes (moving things around) and non-mechanical changes - easier to review / go back in time and figure out where regressions occurred.

@jatgarg
Copy link
Contributor

jatgarg commented Apr 5, 2024

Take a look at some small comments above and you can merge. Thanks.

@vladsud vladsud merged commit e97dcd8 into microsoft:main Apr 5, 2024
31 checks passed
@vladsud vladsud deleted the SnapshotTelemetry branch April 5, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: driver Driver related issues area: odsp-driver base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants