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

ServerTelemetryChannel to not use any storage if configured StorageFolder has issues. #2002

Merged
merged 3 commits into from
Aug 28, 2020

Conversation

cijothomas
Copy link
Contributor

Changes

Changes ServerTelemetryChannel to not fall back to any default folders/directory, if user has provided StorageFolder, and the provided StorageFolder is not usable for any reason (don't exist or no access etc.) Instead an Error level trace message is logged indicating the issue with user configured StorageFolder.

Previously ServerTelemetryChannel uses default directories as a fallback when the user configured StorageFolder fails for any reason. This is incorrect behavior and must be treated as a bug:

  1. Inspecting StorageFolder on the configuration.sink.channel.StorageFolder shows the user configured folder, when SDK is actually using something else. This is misleading.

  2. User might have configured explicit StorageFolder because user did not want SDK to write/read from any default folder. Disregarding user configured folder and reading/writing to a different disk location can be a privacy bug as well. (what if the default folder contains telemetry from a different website in same app pool?)

Checklist

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

  • FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

Notes for reviewers:

  • We support comment build triggers
    • /AzurePipelines run will queue all builds
    • /AzurePipelines run <pipeline-name> will queue a specific build

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@cijothomas cijothomas merged commit c983cc1 into develop Aug 28, 2020
@cijothomas cijothomas deleted the cijothomas/fixstorage1 branch August 28, 2020 22:35
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