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

Updated Calculator diagnostic data collection per the specification #572

Merged
merged 36 commits into from
Jul 18, 2019

Conversation

sanderl
Copy link
Contributor

@sanderl sanderl commented Jun 27, 2019

Fixes #525: Refresh Calculator Diagnostic Data.

Description of the changes:

  • Removed unneeded diagnostic events and code
  • Added and consolidated events into the events defined in the spec

How changes were validated:

  • Tested manually to ensure the diagnostic events were logged as expected

sanderl and others added 24 commits May 14, 2019 11:19
…ixed issue where button click telemetry is sent per window close
…ed out telemetry points. Cleaned up the TraceLogger.cpp and TraceLogger.h files to remove unused fields and methods.
@sanderl sanderl requested a review from EriWong June 27, 2019 18:28
@HowardWolosky HowardWolosky changed the title Updated Calculator Telemetry per the Calculator Diagnostics Spec. Updated Calculator diagnostic data collection per the specification Jun 27, 2019
… size and item clicked rather than the token
Copy link
Member

@mcooley mcooley left a comment

Choose a reason for hiding this comment

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

🕐

@sanderl sanderl requested a review from mcooley July 1, 2019 22:38
@abhiram2466
Copy link

Can you please tell how are you implementing "ButtonUsageInSession" Event?

@sanderl
Copy link
Contributor Author

sanderl commented Jul 11, 2019

Can you please tell how are you implementing "ButtonUsageInSession" Event?

ButtonUsageInSession event will be fired periodically throughout the app. It will send the whole ButtonLog array as a string once the array reaches a size of 10. It also sends the array when the app suspends. Originally I was just sending all of the data on suspend, however there was a potential issue where the data would not be sent on a crash or if the process was killed. Sending the button usage data in batches, mitigates this issue so that even if there is data lost it will only be the last 10 button/mode combinations.

@sanderl sanderl closed this Jul 11, 2019
@sanderl sanderl reopened this Jul 11, 2019
@sanderl sanderl requested a review from EriWong July 11, 2019 17:57
mcooley
mcooley previously approved these changes Jul 16, 2019
Copy link
Member

@mcooley mcooley left a comment

Choose a reason for hiding this comment

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

One change in NumberAndOperatorsEnum and I think this is good to go.

Cube = (int)CM::Command::CommandCUB,
DMS = (int)CM::Command::CommandDMS,
// Enum values below are used for Tracelogging and do not map to the Calculator engine
MemoryAdd,
Copy link
Member

Choose a reason for hiding this comment

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

We should set these to well-known integers--otherwise someone might accidentally change them in the future.

src/CalcViewModel/Common/TraceLogger.cpp Show resolved Hide resolved
src/CalcViewModel/Common/TraceLogger.cpp Show resolved Hide resolved
…ntegers assigned to them. Updated the LogButtonUsage event to only log if ButtonLog is not empty.
@@ -299,7 +301,7 @@ namespace CalculatorApp

void TraceLogger::LogButtonUsage()
{
if (!GetTraceLoggingProviderEnabled())
if (!GetTraceLoggingProviderEnabled() || buttonLog.size() == 0)
Copy link
Member

Choose a reason for hiding this comment

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

This check needs to come after you've taken the lock.

@ghost ghost removed the needs author feedback label Jul 17, 2019
@mcooley
Copy link
Member

mcooley commented Jul 18, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mcooley mcooley merged commit a638426 into microsoft:master Jul 18, 2019
@mcooley mcooley mentioned this pull request Jul 18, 2019
@sanderl sanderl deleted the telemetry branch January 16, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh Calculator Diagnostic Data
7 participants