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

Add hash mismatch telemetry details #4857

Merged
merged 9 commits into from
Oct 11, 2024
Merged

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Oct 7, 2024

Issue

From looking at our hash mismatch data, there were two main modes:

  1. Lots of actual hashes that are the result of hashing a zero-byte file
  2. Other hashes with small numbers of cases, including some non-one counts. This suggests at least some cases with consistent results even if they are not what we expected.

My suspicion is that there are some cases where a successful HTTP response is made, but the content is actually a webpage or similar. This would happen with captive portals or potentially firewalls.

Change

Collect the total size and content type of downloads and add them to our existing hash mismatch telemetry events.
Retry downloading when we get a zero-byte file.
Report a different error when a zero-byte file is the final result of downloading (also requires the hash to not match, so you can still have a zero-byte installer for what that is worth...).

Validation

Updates the downloader tests for new fields.
Adds a new pair of tests for zero-byte file downloads.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner October 7, 2024 23:13
yao-msft
yao-msft previously approved these changes Oct 7, 2024
@@ -151,6 +151,7 @@
#define APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED ((HRESULT)0x8A150083)
#define APPINSTALLER_CLI_ERROR_SFSCLIENT_PACKAGE_NOT_SUPPORTED ((HRESULT)0x8A150084)
#define APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED_FORBIDDEN ((HRESULT)0x8A150085)
#define APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE ((HRESULT)0x8A150086)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: update returnCodes.md ..

@@ -559,6 +563,8 @@ namespace AppInstaller::Logging
TraceLoggingBinary(expected.data(), static_cast<ULONG>(expected.size()), "Expected"),
TraceLoggingBinary(actual.data(), static_cast<ULONG>(actual.size()), "Actual"),
TraceLoggingBool(overrideHashMismatch, "Override"),
TraceLoggingUInt64(downloadSizeInBytes, "FileSize"),
AICLI_TraceLoggingStringView(actualContentType, "ContentType"),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should we update the AICLI_Log below?

Copy link
Contributor

Choose a reason for hiding this comment

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

And summary events too?

@Trenly
Copy link
Contributor

Trenly commented Oct 8, 2024

I see that you’re logging the content type - would there be any value in additional telemetry if the content type is an HTML page? For example - I've noticed that some publishers return the HTML contents of a 403 - Forbidden page on a download attempt, others return the HTML contents of a 404 page. I've also seen some edge cases where the download request results in a 204 - No Content response

I don’t know if any of this would be useful information, but if it is a Zero byte file, perhaps the HTTP status code? If larger than zero bytes and is HTML content, I don’t know if there would be any value in checking the meta tags or searching for 403/404. Just thinking that having additional information on the contents of the webpages could potentially help improve handling in the future. I know that the actual content of the page probably can’t be sent, but information about it such as whether or not it is canonical likely wouldn’t violate any privacy laws?

This comment has been minimized.

@JohnMcPMS JohnMcPMS merged commit 19ce709 into microsoft:master Oct 11, 2024
9 checks passed
@JohnMcPMS JohnMcPMS deleted the hash-telem branch October 11, 2024 21:55
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.

3 participants