Skip to content

Commit 247a73f

Browse files
authored
refactor(gitrest): Add stage traces to the Git write summary flow (#25954)
## Description After some recent difficulties in narrowing down performance issues in the summary flow, this PR takes the first step to making that easier in the future by adding stage traces to the summary flow. This will allow us to more easily analyze where a problem is occurring. ## Reviewer Guidance Bumped server package versions to consume newly shared `StageTrace` from services-core
1 parent 1e75326 commit 247a73f

File tree

7 files changed

+164
-85
lines changed

7 files changed

+164
-85
lines changed

server/gitrest/packages/gitrest-base/package.json

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,15 @@
5252
},
5353
"dependencies": {
5454
"@fluidframework/common-utils": "^1.1.1",
55-
"@fluidframework/gitresources": "6.0.0-340759",
56-
"@fluidframework/protocol-base": "6.0.0-340759",
55+
"@fluidframework/gitresources": "8.0.0-366087",
56+
"@fluidframework/protocol-base": "8.0.0-366087",
5757
"@fluidframework/protocol-definitions": "^3.2.0",
58-
"@fluidframework/server-services-client": "6.0.0-340759",
59-
"@fluidframework/server-services-core": "6.0.0-340759",
60-
"@fluidframework/server-services-shared": "6.0.0-340759",
61-
"@fluidframework/server-services-telemetry": "6.0.0-340759",
62-
"@fluidframework/server-services-utils": "6.0.0-340759",
63-
"@fluidframework/server-test-utils": "6.0.0-340759",
58+
"@fluidframework/server-services-client": "8.0.0-366087",
59+
"@fluidframework/server-services-core": "8.0.0-366087",
60+
"@fluidframework/server-services-shared": "8.0.0-366087",
61+
"@fluidframework/server-services-telemetry": "8.0.0-366087",
62+
"@fluidframework/server-services-utils": "8.0.0-366087",
63+
"@fluidframework/server-test-utils": "8.0.0-366087",
6464
"async-mutex": "^0.3.2",
6565
"axios": "^1.8.4",
6666
"body-parser": "^1.20.3",

server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
NetworkError,
1010
isNetworkError,
1111
} from "@fluidframework/server-services-client";
12+
import { StageTrace } from "@fluidframework/server-services-core";
1213
import { Lumberjack } from "@fluidframework/server-services-telemetry";
1314

1415
import type { IRepositoryManager, IFileSystemManager } from "./definitions";
@@ -18,6 +19,7 @@ import {
1819
Constants,
1920
type ISummaryWriteFeatureFlags,
2021
type IWriteSummaryInfo,
22+
WriteSummaryTraceStage,
2123
isChannelSummary,
2224
isContainerSummary,
2325
readSummary,
@@ -106,6 +108,9 @@ export class GitWholeSummaryManager {
106108
GitRestLumberEventName.WholeSummaryManagerWriteSummary,
107109
lumberjackProperties,
108110
);
111+
const writeSummaryTrace = new StageTrace<WriteSummaryTraceStage>(
112+
WriteSummaryTraceStage.WriteSummaryStarted,
113+
);
109114
try {
110115
if (isChannelSummary(payload)) {
111116
lumberjackProperties.summaryType = "channel";
@@ -119,7 +124,9 @@ export class GitWholeSummaryManager {
119124
lumberjackProperties,
120125
},
121126
this.summaryWriteFeatureFlags,
127+
writeSummaryTrace,
122128
);
129+
writeSummaryMetric.setProperty("summaryTrace", writeSummaryTrace);
123130
writeSummaryMetric.setProperty("treeSha", writeSummaryInfo.writeSummaryResponse.id);
124131
writeSummaryMetric.success(
125132
"GitWholeSummaryManager succeeded in writing channel summary",
@@ -140,6 +147,7 @@ export class GitWholeSummaryManager {
140147
},
141148
this.summaryWriteFeatureFlags,
142149
);
150+
writeSummaryMetric.setProperty("summaryTrace", writeSummaryTrace);
143151
writeSummaryMetric.setProperty("newDocument", writeSummaryInfo.isNew);
144152
writeSummaryMetric.setProperty(
145153
"commitSha",
@@ -156,6 +164,7 @@ export class GitWholeSummaryManager {
156164
});
157165
throw new NetworkError(400, `Unknown Summary Type: ${payload.type}`);
158166
} catch (error: any) {
167+
writeSummaryMetric.setProperty("summaryTrace", writeSummaryTrace);
159168
writeSummaryMetric.error("GitWholeSummaryManager failed to write summary", error);
160169
throw error;
161170
}

server/gitrest/packages/gitrest-base/src/utils/wholeSummary/definitions.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,38 @@ export interface IFullGitTree {
4848
*/
4949
parsedFullTreeBlobs: boolean;
5050
}
51+
52+
export enum WriteSummaryTraceStage {
53+
/**
54+
* Summary write operation started, no logic executed yet.
55+
*/
56+
WriteSummaryStarted = "WriteSummaryStarted",
57+
/**
58+
* Retrieved document's Git Ref from storage (if it existed).
59+
* This is a no-op in some scenarios, like on initial summary.
60+
*/
61+
DocRefRetrieved = "DocRefRetrieved",
62+
/**
63+
* Computed the Git tree entries for the summary tree to be written.
64+
* This includes performing any "low IO" logic if enabled.
65+
*/
66+
ComputedTreeEntries = "ComputedTreeEntries",
67+
/**
68+
* Successfully wrote the summary tree to Git storage.
69+
* This includes writing any blobs and tree nodes to storage.
70+
*/
71+
WroteSummaryTree = "WroteSummaryTree",
72+
/**
73+
* Created a new commit referencing the written summary tree.
74+
*/
75+
CreatedNewSummaryVersion = "CreatedNewSummaryVersion",
76+
/**
77+
* Created or updated the document Git Ref to point to the new commit.
78+
*/
79+
CreatedOrUpdatedDocRef = "CreatedOrUpdatedDocRef",
80+
/**
81+
* Converted the full Git tree into a Whole Flat Summary structure to be returned
82+
* to the caller.
83+
*/
84+
ConvertedToWholeFlatSummary = "ConvertedToWholeFlatSummary",
85+
}

server/gitrest/packages/gitrest-base/src/utils/wholeSummary/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
export { Constants } from "./constants";
7+
export { WriteSummaryTraceStage } from "./definitions";
78
export { readSummary } from "./readWholeSummary";
89
export {
910
ISummaryWriteFeatureFlags,

server/gitrest/packages/gitrest-base/src/utils/wholeSummary/writeWholeSummary.ts

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
IWriteSummaryResponse,
1111
WholeSummaryTreeEntry,
1212
} from "@fluidframework/server-services-client";
13+
import { StageTrace } from "@fluidframework/server-services-core";
1314
import { Lumberjack } from "@fluidframework/server-services-telemetry";
1415

1516
import { GitRestLumberEventName } from "../gitrestTelemetryDefinitions";
@@ -19,7 +20,12 @@ import {
1920
type IWriteSummaryTreeOptions,
2021
writeSummaryTree as writeSummaryTreeCore,
2122
} from "./coreWriteUtils";
22-
import type { IFullGitTree, ISummaryVersion, IWholeSummaryOptions } from "./definitions";
23+
import {
24+
IFullGitTree,
25+
ISummaryVersion,
26+
IWholeSummaryOptions,
27+
WriteSummaryTraceStage,
28+
} from "./definitions";
2329
import { computeLowIoSummaryTreeEntries } from "./lowIoWriteUtils";
2430

2531
/**
@@ -127,6 +133,7 @@ async function writeSummaryTree(
127133
payload: IWholeSummaryPayload,
128134
documentRef: IRef | undefined,
129135
options: IWholeSummaryOptions & { precomputeFullTree: boolean; useLowIoWrite: boolean },
136+
stageTrace?: StageTrace<WriteSummaryTraceStage>,
130137
): Promise<IFullGitTree> {
131138
const writeSummaryTreeOptions: IWriteSummaryTreeOptions = {
132139
repoManager: options.repoManager,
@@ -145,6 +152,7 @@ async function writeSummaryTree(
145152
writeSummaryTreeOptions,
146153
options,
147154
);
155+
stageTrace?.stampStage(WriteSummaryTraceStage.ComputedTreeEntries);
148156

149157
const writeSummaryTreeMetric = Lumberjack.newLumberMetric(
150158
GitRestLumberEventName.WriteSummaryTree,
@@ -168,15 +176,23 @@ export async function writeChannelSummary(
168176
payload: IWholeSummaryPayload,
169177
options: IWholeSummaryOptions,
170178
featureFlags: ISummaryWriteFeatureFlags,
179+
stageTrace?: StageTrace<WriteSummaryTraceStage>,
171180
): Promise<IWriteSummaryInfo> {
172181
const useLowIoWrite = featureFlags.enableLowIoWrite === true;
173182
// We need the document Ref to write channel with LowIo so that we can access pointers.
174183
const documentRef: IRef | undefined = useLowIoWrite ? await getDocRef(options) : undefined;
175-
const fullGitTree = await writeSummaryTree(payload, documentRef, {
176-
...options,
177-
precomputeFullTree: false,
178-
useLowIoWrite,
179-
});
184+
stageTrace?.stampStage(WriteSummaryTraceStage.DocRefRetrieved);
185+
const fullGitTree = await writeSummaryTree(
186+
payload,
187+
documentRef,
188+
{
189+
...options,
190+
precomputeFullTree: false,
191+
useLowIoWrite,
192+
},
193+
stageTrace,
194+
);
195+
stageTrace?.stampStage(WriteSummaryTraceStage.WroteSummaryTree);
180196
return {
181197
isNew: false,
182198
writeSummaryResponse: {
@@ -291,23 +307,31 @@ export async function writeContainerSummary(
291307
isInitial: boolean,
292308
options: IWholeSummaryOptions,
293309
featureFlags: ISummaryWriteFeatureFlags,
310+
stageTrace?: StageTrace<WriteSummaryTraceStage>,
294311
): Promise<IWriteSummaryInfo> {
295312
// Ref will not exist for an initial summary, so do not bother checking.
296313
const documentRef: IRef | undefined =
297314
featureFlags.optimizeForInitialSummary && isInitial === true
298315
? undefined
299316
: await getDocRef(options);
317+
stageTrace?.stampStage(WriteSummaryTraceStage.DocRefRetrieved);
300318
const isNewDocument = !documentRef && payload.sequenceNumber === 0;
301319
const useLowIoWrite =
302320
featureFlags.enableLowIoWrite === true ||
303321
(isNewDocument && featureFlags.enableLowIoWrite === "initial");
304322

305323
// Write the container summary tree into storage.
306-
const fullGitTree = await writeSummaryTree(payload, documentRef, {
307-
...options,
308-
precomputeFullTree: true,
309-
useLowIoWrite,
310-
});
324+
const fullGitTree = await writeSummaryTree(
325+
payload,
326+
documentRef,
327+
{
328+
...options,
329+
precomputeFullTree: true,
330+
useLowIoWrite,
331+
},
332+
stageTrace,
333+
);
334+
stageTrace?.stampStage(WriteSummaryTraceStage.WroteSummaryTree);
311335

312336
// Create a new commit referencing the container summary tree.
313337
const { id: versionId, treeId } = await createNewSummaryVersion(
@@ -317,11 +341,14 @@ export async function writeContainerSummary(
317341
payload.sequenceNumber,
318342
options,
319343
);
344+
stageTrace?.stampStage(WriteSummaryTraceStage.CreatedNewSummaryVersion);
320345

321346
// Create or update the document ref to reference the new commit.
322347
await createOrUpdateRef(documentRef, versionId, options);
348+
stageTrace?.stampStage(WriteSummaryTraceStage.CreatedOrUpdatedDocRef);
323349

324350
const fullSummaryTree = convertFullGitTreeToFullSummaryTree(fullGitTree);
351+
stageTrace?.stampStage(WriteSummaryTraceStage.ConvertedToWholeFlatSummary);
325352
const wholeFlatSummary: IWholeFlatSummary = {
326353
id: versionId,
327354
trees: [

server/gitrest/packages/gitrest/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@
2828
},
2929
"dependencies": {
3030
"@fluidframework/gitrest-base": "workspace:~",
31-
"@fluidframework/server-services-shared": "6.0.0-340759",
32-
"@fluidframework/server-services-utils": "6.0.0-340759",
31+
"@fluidframework/server-services-shared": "8.0.0-366087",
32+
"@fluidframework/server-services-utils": "8.0.0-366087",
3333
"body-parser": "^1.20.3",
3434
"compression": "^1.7.3",
3535
"cors": "^2.8.5",

0 commit comments

Comments
 (0)