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

Disable User_Id sampling to better support cross-component correlation and tracing #625

Closed
Dmitry-Matveev opened this issue Sep 19, 2017 · 0 comments

Comments

@Dmitry-Matveev
Copy link
Member

Calculation of the sampling score based on user_Id field should be disabled by default to ensure in successful correlation across many services on operation_Id field.

With the current implementation, user_Id is not propagated further than the first backend server. That means that first components in the app architecture are sampled based on user_Id while all others are sampled on operation_Id. This leads to inability to provide a distributed trace for the most of the transactions. An attempts to propagate user_Id beyond the cookie in the client-server relationship is a privacy concern as it would expose user behavior (although, not any of the user info) to the application's external dependencies, not only to the next internal service in the chain.

Note: While disabling user_Id sampling fixed cross-component correlation and distributed tracing from the sampling perspective, it actually affects several scenarios we might rely on in UI and it requires a discussion to agree on the right approach with this change.

For instance, switching from the user_Id sampling to operation_Id sampling in all server-side SDKs will lead to the partial info displayed in this UI:
image

Switching in JavaScript SDK will lead to issues with the use session event tracking unless customer events and page views are exempt of the sampling.

Making this switch between user_Id and operation_Id configurable is considered in #221 . This might allow customers to override the behavior for the purposes of sampling full info about the certain users rather than certain operation for the user. However, this option will require a careful privacy review and notes for the customers.

@Dmitry-Matveev Dmitry-Matveev modified the milestones: 2.5-Beta1, 2.5-Beta2 Sep 19, 2017
d3r3kk added a commit that referenced this issue Nov 16, 2017
- Moved the configuration out as far as the AdaptiveSamplingTelemetryProcessor
- Added unit test to ensure sampling with/without user id is tested
- Updated changelog for beta 2.5.0-beta2
- Updated PublicAPI.Unshipped.txt

Linked to issue #625
TimothyMothra added a commit that referenced this issue Dec 4, 2017
* apply patches from source build

* remove legacy file

* Fix null check to use string.IsNullOrEmpty. Even though unlikely, if a string.empty is passed as input it will result in an infinite loop.

* increase the version to 2.5.0-beta2

* The issue with compareNetObject got resolved

* Update Microsoft.ApplicationInsights.netcoreapp11.Tests.csproj

* GH items updated

* dirs.proj to do solution restore.

* Remove sampling score on telemetry items related to Context.User.Id

Issue #625

* Update Changelog

* Add link to CHANGELOG regarding issue

* Update PULL_REQUEST_TEMPLATE.md

* Update PULL_REQUEST_TEMPLATE.md

* Test implementation for Linux vs Windows

* ApplicationFolderProvider modified to work better in non-windows.
API Surface updated.

* Minor renaming, build fixes.

* UnitTest fix and addition of new tests

* Error messages updated

* Enable building on mac

* disabled non-netstandard targets

* clean up projects

* clean up projects

* fix it back

* sln from dirs

* test task

* renamings

* Smaller changes and updating GH templates

* remove public api changes. code change to follow this.

* PR suggestion incorporated - removed flg to override storage behaviour

* Minor

* Correctmessage

* Documentation

* better error messages.

* readyonly IIdentityProvider

* update changelog

* changelog details

* Revert references to MetricManager.
Not ready for public use.

* Proposed fix for issue microsoft/ApplicationInsights-dotnet-server#756.

-Add an internal constructor to `SamplingTelemetryProcessor` that gives it a next processor for sampled, and a next processor for unsampled items.
- In the `AdaptiveSamplingTelemetryProcessor`, give the `SamplingPercentageEstimatorTelemetryProcessor` as well as the actual next `ITelemetryProcessor` in the pipeline to the `SamplingTelemetryProcessor`'s internal constructor.
- Add property `SamplingTelemetryProcessor.UnsampledNext` to send to the next `ITelemetryProcessor` in the pipeline, bypassing the estimator which usually comes immediately next in the chain.
- Enhance constructor for `SamplingTelemetryProcessor` to set the `Next` and `UnsampledNext` fields based on which constructor was called. (`UnsampledNext` == `Next` when calling the public constructor only).
- `SamplingTelemetryProcessor.Process(ITelemetry item)` updated to call the appropriate `Next`/`UnsampledNext` property.
- Remove an unused using.
- Update CHANGELOG.md with the fix info

* PR Suggestions and further fix

- Renamed Next to SampledNext
- Unnecessary constructor logic removed
- Comments fixed up (remove unnecessary commenting)
- Early exit from processing removed when SampledNext != UnsampledNext, leaving original intent intact

* Add a few tests to ensure assumptions during AdaptiveSampling are maintained

- Ensure Process sends telemetry items to the correct '...Next' method while being passed through the SamplingTelemetryProcessor

- Ensure past behaviour on the standalone (not part of Adaptive sampling chain) SamplingTelemetryProcessor is maintained

* Initial PR for HealthHeartbeat (#636)

Add Heartbeat feature to Application Insights SDK

- [x] Changes in public surface reviewed
- [x] Design discussion issue #628  
- [x] CHANGELOG.md updated

- Add all unit tests necessary to comform to the current stated feature spec
- Add API updates to the Unshipped txt file
- Add remark (still with TODO as I have yet to implement) about all default payload members
- Changes in design from polling providers that will extend heartbeat payloads to expecting consumers of the SDK to 'push' state changes as necessary.
- Add/Set methods to handle field collision management
- Expose new property 'IsHeartbeatEnabled' to turn on/off heartbeats
- Changelog updated for beta2
- create HeartbeatProvider with DiagnosticsTelemetryProvider to enable simpler initialization / logic requirements
- Increase default heartbeat interval to 15 minutes
- Documentation on how to extend Heartbeat properties

* Update Changelog, prep for 2.5.0-beta2 (#664)
@d3r3kk d3r3kk closed this as completed Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants