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.

Sign up for GitHub

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

Jump to bottom

Update Telemetry for Public Preview in MSAZ #251

Merged
merged 14 commits into from
Oct 29, 2024
Merged

Update Telemetry for Public Preview in MSAZ #251

merged 14 commits into from
Oct 29, 2024

Conversation

NoriZC
Copy link
Contributor

@NoriZC NoriZC commented Oct 22, 2024

PR Summary

  • Remove HistoryMessage and Duration from Telemetry
  • Migrate Application Insight from temp to test
  • Add replace to AzCLI telemetry

PR Context

@NoriZC NoriZC mentioned this pull request Oct 22, 2024
Closed
daxian-dbw
daxian-dbw requested changes Oct 22, 2024
View reviewed changes
shell/agents/Microsoft.Azure.Agent/ChatSession.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/AzureAgent.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/AzureAgent.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/AzureAgent.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/AzureAgent.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/Telemetry/MetricHelper.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/Telemetry/MetricHelper.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/Telemetry/MetricHelper.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/ChatSession.cs Outdated Show resolved Hide resolved
shell/agents/Microsoft.Azure.Agent/Telemetry/MetricHelper.cs Outdated Show resolved Hide resolved
@daxian-dbw
Copy link
Member

Please search for "\todo telemetry" in the code base. We need telemetry for those too.

Also please sync this branch before you make any further changes, because I have rebased it with the main branch.

@NoriZC
Copy link
Contributor Author

NoriZC commented Oct 25, 2024

@daxian-dbw Thanks for comments~ Addressed all of them now. Please help review again when you get a chance.

@NoriZC @daxian-dbw
telemetry for msaz
d7a00ce 
telemetry for msaz

Remove unneeded changes to `CopilotResponse`

Address Comments

Minor fix

set json serializer option
daxian-dbw
daxian-dbw requested changes Oct 29, 2024
View reviewed changes
shell/agents/Microsoft.Azure.Agent/Telemetry.cs Outdated
{
string azureConfigDir = Environment.GetEnvironmentVariable("AZURE_CONFIG_DIR");
string userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
string userProfilePath = Path.Combine(string.IsNullOrEmpty(azureConfigDir) ? userProfile : azureConfigDir, "azureProfile.json");
Copy link
Member

Choose a reason for hiding this comment

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

I don't have the env variable AZURE_CONFIG_DIR defined, and also don't have either the file C:\Users\dongbow\azureProfile.json or the file C:\Users\dongbow\AzureRmContextSettings.json. Are you sure we should look at those files and they are the right paths?

Also, we are using AzCLI login token today and the Az PowerShell login token doesn't work. In this case, aren't we supposed to use the AzCLI profile instead?

Copy link
Contributor Author

@NoriZC NoriZC Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I made stupid mistakes here for the first issue. Please try again!!

Azure CLI and Azure PS share the same installation id. File azureconfig.json contains the id generated by AzCLI Installation, and AzureRmContextSettings.json is generated by AzPS Installation. If we are supposed to support Az PowerShell as individual dependency when in the future the login token is good, we can keep current code.

daxian-dbw
daxian-dbw requested changes Oct 29, 2024
View reviewed changes
shell/agents/Microsoft.Azure.Agent/Telemetry.cs Outdated
{
try
{
Path.Combine(string.IsNullOrEmpty(azureConfigDir) ? userProfile : azureConfigDir, "AzureRmContextSettings.json");
Copy link
Member

@daxian-dbw daxian-dbw Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You didn't assign this value to anything ...
You got to review your own changes before pushing a commit.

daxian-dbw
daxian-dbw reviewed Oct 29, 2024
View reviewed changes
shell/agents/Microsoft.Azure.Agent/Telemetry.cs Outdated
{
string azureConfigDir = Environment.GetEnvironmentVariable("AZURE_CONFIG_DIR");
string userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
string userProfilePath = string.IsNullOrEmpty(azureConfigDir) ? Path.Combine(userProfile, ".Azure", "azureProfile.json") : azureConfigDir;
Copy link
Member

Choose a reason for hiding this comment

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

This is still wrong:

  1. The folder is .azure, all in lower case. .Azure will not work on non-windows.
  2. When azureConfigDir is set, userProfilePath will be set to that folder, not to a file.

Copy link
Member

Choose a reason for hiding this comment

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

I have rewritten this piece.

shell/agents/Microsoft.Azure.Agent/Telemetry.cs Outdated
{
try
{
userProfilePath = string.IsNullOrEmpty(azureConfigDir) ? Path.Combine(userProfile, ".Azure", "AzureRmContextSettings.json") : azureConfigDir;
Copy link
Member

Choose a reason for hiding this comment

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

Same here. When azureConfigDir is set, userProfilePath will be set to that folder, not to a file.

Copy link
Member

Choose a reason for hiding this comment

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

I have rewritten this piece.

shell/agents/Microsoft.Azure.Agent/Telemetry.cs Outdated
};

_telemetryClient.TrackTrace("AIShell", telemetryEvent);
if (e != null) { _telemetryClient.TrackException(e); }
Copy link
Member

@daxian-dbw daxian-dbw Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How do you corelate the exception e with the telemetryEvent?
Also, we shouldn't call Flush per every telemetry event, see https://devblogs.microsoft.com/premier-developer/application-insights-use-case-for-telemetryclient-flush-calls/

Copy link
Member

Choose a reason for hiding this comment

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

I have fixed it.

daxian-dbw
daxian-dbw approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@NoriZC Things seem to be working in my manual tests. Can you please review all the change and also do your testing on the telemetry? Thanks

[Update] I need to have this PR merged to unblock the changes about plugin-store. But please do test the changes on your side to make sure the telemetries are captured as expected. If anything doesn't work, please submit and PR and let me know. Thanks

@daxian-dbw daxian-dbw merged commit da3e6d5 into PowerShell:main Oct 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@StevenBucher98 StevenBucher98 Awaiting requested review from StevenBucher98

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NoriZC @daxian-dbw @StevenBucher98