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

refactor: add telemetry for office agent #11831

Merged
merged 23 commits into from
Jul 2, 2024

Conversation

knightlea
Copy link

const purifiedResult = await getCopilotResponseAsString(
"copilot-gpt-4",
purifyUserMessage,
token
);
const t1 = performance.now();
const requestTokens = countMessagesTokens(purifyUserMessage);
const responseTokens = Tokenizer.getInstance().tokenLength(purifiedResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use countMessagesTokens here. Same for the rest.

Copy link
Author

Choose a reason for hiding this comment

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

done, thanks

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 89.40%. Comparing base (557dab3) to head (e2ccfc1).
Report is 39 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #11831      +/-   ##
==========================================
+ Coverage   89.34%   89.40%   +0.05%     
==========================================
  Files         566      569       +3     
  Lines       33150    33311     +161     
  Branches     6417     6541     +124     
==========================================
+ Hits        29619    29781     +162     
+ Misses       1612     1601      -11     
- Partials     1919     1929      +10     
Files Coverage Δ
...extension/src/officeChat/commands/create/helper.ts 99.09% <100.00%> (+0.02%) ⬆️
.../commands/nextStep/officeNextstepCommandHandler.ts 96.61% <100.00%> (+0.31%) ⬆️
...on/src/officeChat/common/samples/sampleProvider.ts 92.63% <100.00%> (+0.18%) ⬆️
...sion/src/officeChat/common/skills/codeExplainer.ts 100.00% <100.00%> (ø)
...src/officeChat/common/skills/codeIssueCorrector.ts 96.40% <100.00%> (+0.05%) ⬆️
...-extension/src/officeChat/common/skills/printer.ts 100.00% <100.00%> (ø)
...ion/src/officeChat/common/skills/projectCreator.ts 100.00% <ø> (ø)
...ode-extension/src/officeChat/common/skills/spec.ts 100.00% <100.00%> (ø)
packages/vscode-extension/src/officeChat/utils.ts 100.00% <100.00%> (ø)
...code-extension/src/telemetry/extTelemetryEvents.ts 100.00% <100.00%> (ø)
... and 6 more

... and 11 files with indirect coverage changes

OffTopic = "Off Topic",
LanguageModelError = "LanguageModel Error",
}
export class OfficeChatTelemetryData extends ChatTelemetryData {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are implementing all member variables/methods, please implement the interface IChatTelemetryData instead of extend ChatTelemetryData

Copy link
Author

Choose a reason for hiding this comment

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

will fix, thanks

@@ -95,11 +112,11 @@ export function getOfficeTemplateMetadata(): ProjectMetadata[] {
export async function showOfficeSampleFileTree(
projectMetadata: ProjectMetadata,
response: ChatResponseStream
): Promise<string> {
): Promise<object> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Define a object struct { path: string, host: string } instead of returning an object

Copy link
Author

Choose a reason for hiding this comment

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

will fix, thanks

Comment on lines 81 to 83
const sampleInfos = await showOfficeSampleFileTree(matchedResult, response);
const folder = (sampleInfos as any)?.["path"];
const hostType = (sampleInfos as any)?.["host"].toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the defined object structure to fetch these variables instead of cast to any

Copy link
Author

Choose a reason for hiding this comment

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

will fix, thanks

const sampleTitle = localize("teamstoolkit.chatParticipants.create.sample");
officeChatTelemetryData.setHostType(hostType);
const matchResultInfo = "sample";
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been defined in matchedResult.type, remove this variable.

Copy link
Author

Choose a reason for hiding this comment

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

will fix, thanks

Comment on lines 93 to 94
const tmpHostType = (matchedResult.data as any)?.["addin-host"].toLowerCase();
const tmpFolder = await showOfficeTemplateFileTree(matchedResult.data as any, response);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, use the defined object struct.

Copy link
Author

Choose a reason for hiding this comment

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

the type of matchedResult.data is unknown so that I think it should be convert to any

const templateTitle = localize("teamstoolkit.chatParticipants.create.template");
officeChatTelemetryData.setHostType(tmpHostType);
const tmpmatchResultInfo = "template";
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove and use matchedResult.type

Copy link
Author

Choose a reason for hiding this comment

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

will fix, thanks

response.markdown(`
${localize("teamstoolkit.chatParticipants.officeAddIn.printer.outputTemplate.intro")}\n
${purified}
`);
const spec = new Spec(purified);
spec.appendix.telemetryData.requestId = telemetryData.requestId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put it into the constructor

Copy link
Author

Choose a reason for hiding this comment

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

will fix, thanks

Comment on lines 150 to 152
// telemetryData.setCodeClassAndMembers(
// spec.appendix.telemetryData.codeClassAndMembers.toString()
// );
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these lines

Copy link
Author

Choose a reason for hiding this comment

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

will remove, thanks

Comment on lines 53 to 60
const t1 = performance.now();
telemetryData.responseTokensPerRequest.push(
OfficeChatTelemetryData.calculateResponseTokensPerRequest(response, t0, t1)
);
telemetryData.chatMessages.push(
...messages,
new LanguageModelChatMessage(LanguageModelChatMessageRole.Assistant, response)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe log the response tokens/LLM call is unnecessary. We can only log all response tokens in an array and calculate this metrics in markComplete. Please feel free to call out if you have other thoughts.

Copy link
Author

Choose a reason for hiding this comment

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

have fixed to distinguish request token and response token and removed response tokens per request

Comment on lines 136 to 137
// this.telemetryData.properties[TelemetryProperty.CopilotChatCodeClassAndMembers] =
// this.codeClassAndMembers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these lines

Copy link
Author

Choose a reason for hiding this comment

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

will remove, thanks

CopilotMatchResultType = "copilot-match-result-type",
CopilotChatBlockReason = "copilot-chat-block-reason",
CopilotChatRelatedSampleName = "copilot-chat-related-sample-name",
CopilotChatCodeClassAndMembers = "copilot-chat-code-class-and-members",
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this unused variable

Copy link
Author

Choose a reason for hiding this comment

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

done, thanks

CopilotChatTimeToFirstToken = "copilot-chat-time-to-first-token",
CopilotChatTotalTokensPerSecond = "copilot-chat-total-tokens-per-second",
CopilotChatResponseTokensPerRequest = "copilot-chat-response-tokens-per-request",
CopilotChatTotalTokens = "copilot-chat-total-tokens",
Copy link
Contributor

@GavinGu07 GavinGu07 Jun 26, 2024

Choose a reason for hiding this comment

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

There is existing CopilotChatTokenCount, so we should just use that.

Copy link
Author

@knightlea knightlea Jun 27, 2024

Choose a reason for hiding this comment

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

the existing name could not distinguish request token and response token. I have added 2 new telemetry names to make it clear

@@ -102,5 +123,5 @@ export async function getOfficeSampleDownloadUrlInfo(sampleId: string) {
if (!sample) {
throw new Error("Sample not found");
}
return sample.downloadUrlInfo;
return { downloadUrlInfo: sample.downloadUrlInfo, host: sample.types[0] };
Copy link
Contributor

Choose a reason for hiding this comment

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

Change the function name to getOfficeSample and return the sample object in this method. Fetch the downloadUrlInfo and host type in outer calling function.

Copy link
Author

Choose a reason for hiding this comment

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

done, thanks

Comment on lines 411 to 424
// groupedMethodsOrProperties.forEach((methodsOrPropertiesCandidates, className) => {
// for (let i = 0; i < methodsOrPropertiesCandidates.length; i++) {
// let methodOrProperty = methodsOrPropertiesCandidates[i].codeSample;
// if (methodOrProperty.startsWith("readonly ")) {
// methodOrProperty = methodOrProperty.replace("readonly ", "");
// }
// const lastColonIndex = methodOrProperty.lastIndexOf(":");
// if (lastColonIndex !== -1) {
// methodOrProperty = methodOrProperty.substring(0, lastColonIndex);
// }
// const classCode = `${className}.${methodOrProperty}`;
// spec.appendix.telemetryData.codeClassAndMembers.push(classCode);
// }
// });
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these lines if never used.

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -367,6 +383,7 @@ ${spec.appendix.codeExplanation
// host,
// codeSpec,
// "" //sampleCode
// spec
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line if never used.

Copy link
Author

@knightlea knightlea Jun 27, 2024

Choose a reason for hiding this comment

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

I have also changed the function getMostRelevantDeclarationsUsingLLM because there is an interaction with copilot in it. seems like there is no other usage of the function in other code. should I also remove my change on the function?

@@ -40,8 +41,8 @@ export default async function officeCreateCommandHandler(

if (request.prompt.trim() === "") {
response.markdown(localize("teamstoolkit.chatParticipants.officeAddIn.create.noPromptAnswer"));

officeChatTelemetryData.markComplete();
officeChatTelemetryData.setBlockReason("Empty Input");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the 'Empty Input' be set in an Enum?

Copy link
Author

Choose a reason for hiding this comment

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

yes I missed it. will fix, thanks

@@ -11,3 +11,8 @@ export interface ICopilotChatOfficeResultMetadata {
export interface ICopilotChatOfficeResult extends ChatResult {
readonly metadata?: ICopilotChatOfficeResultMetadata;
}

export type ProjectMiniData = {
Copy link
Author

Choose a reason for hiding this comment

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

Sure, thanks

response.markdown(
"\nWe've found a sample project that matches your description. Take a look at it below."
);
const downloadUrlInfo = await getOfficeSampleDownloadUrlInfo(projectMetadata.id);
const { downloadUrlInfo, host } = await getOfficeSampleDownloadUrlInfo(projectMetadata.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the host type of the matched templates? seems like there's only telemetry for matched samples.

Copy link
Author

Choose a reason for hiding this comment

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

it has been already logged. thanks

Comment on lines 103 to 105
if (spec.appendix.telemetryData.isHarmful === false) {
telemetryData.setBlockReason(OfficeChatTelemetryBlockReasonEnum.RAI);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be set when isHamrful === true?

@1openwindow 1openwindow merged commit 88458b9 into OfficeDev:dev Jul 2, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants