Fixing FixedRateSampling to work in old and new version of sampling #540

anandsugumaran · 2018-01-29T11:12:55Z

Fixed both the below issues in this commit.

New version of FixedRate Sampling worked only with global sampling percentage and not with user set sampling percentage.
Old version of FixedRate Sampling sets sampling score as SamplingPercentage once sampled in. It has to be sampling percentage.

msftclas · 2018-01-29T11:13:05Z

All CLA requirements met.

anandsugumaran · 2018-01-29T11:25:24Z

...a/com/microsoft/applicationinsights/internal/channel/sampling/FixedRateTelemetrySampler.java


                        return false;
                    }
-                    samplingSupportingTelemetry.setSamplingPercentage(telemetrySamplingScore);


telemetrySamplingScore is a dynamically generated value based on operation id. This cannot be set as Sampling percentage after sampled in since this will affect the item count value in the analytics portal. So replaced this with testedPercentage which will have the correct sampling percentage.

anandsugumaran · 2018-01-29T11:27:22Z

...oft/applicationinsights/internal/channel/samplingV2/FixedRateSamplingTelemetryProcessor.java


                    samplingSupportingTelemetry.setSamplingPercentage(samplingPercentage);
-
-                    if (SamplingScoreGeneratorV2.getSamplingScore(telemetry) >= samplingPercentage) {


As per existing code , sampling will work only when the user didn't set the sampling percentage. To fix this moved the code which samples the telemetry out of the if condition.

anandsugumaran · 2018-01-29T11:28:35Z

...oft/applicationinsights/internal/channel/samplingV2/FixedRateSamplingTelemetryProcessor.java

                    InternalLogger.INSTANCE.info("Item has sampling percentage already set to :"
                            + samplingSupportingTelemetry.getSamplingPercentage());
+
+                    samplingPercentage = samplingSupportingTelemetry.getSamplingPercentage();


Override the local sampling percentage with the user set sampling percentage. Else global sampling percentage will be applied even if the user has set the sampling percentage.

anandsugumaran

Please review the changes done to fix the issues in FixedRateSampling

dhaval24 · 2018-01-29T21:31:38Z

@anandsugumaran thank you very much for your valuable contribution. We would review it and let you know if there needs to be any changes.

dhaval24

Changes look fine to me. I agree that when user programatically sets the sampling percentage it should be honored to the highest priority. I will let others also review this before we merge into.

anandsugumaran · 2018-02-02T08:05:42Z

@littleaj @grlima Can you please review this PR ?

littleaj

Looks good. Thanks for your contribution!

dhaval24 · 2018-02-02T19:09:49Z

@anandsugumaran by the way just was curious, were you able to do an E2E testing with your change and able to see the right item counts on the Analytics Portal? We would like to analyze that before merging this, and ofcourse as @littleaj said thank you very much for your valuable contribution.

anandsugumaran · 2018-02-03T09:35:29Z

@dhaval24 yeah I did and it looked good in the portal as well.

anandsugumaran · 2018-02-08T16:56:03Z

@dhaval24 can you merge the changes if it's looking good

dhaval24 · 2018-02-08T17:03:24Z

@anandsugumaran sure, we will do that by the end of the day.

dhaval24 · 2018-02-09T17:11:57Z

@anandsugumaran thank you very much for your contribution. We have merged your changes and it would likely be the part of 2.1.0 version of our SDK (Which is one release after the stable 2.0.0 version)

…540) Overriding default sampling percentage when programatically specified sampling percentage by user.

dhaval24 · 2018-02-17T00:30:04Z

@anandsugumaran your changes are being taken to stable release of our SDK which would happen soon. Hopefully this would unblock your team.

anandsugumaran · 2018-02-17T09:28:01Z

@dhaval24 Thanks Dhaval.

@dhaval24

* Fix null ref check in telemetry correlation Utils (#541) * Fix null ref check in TelemetryCorrelationUtils * Modifying log level to warning * Updating Changelog * Fix handling of NaN and +/-Infinity in JSON serializer (#499) * Handle NaN and +/-Infinity in metrics * Default NaN/Infinity serialization to 0 to be consistent with other AI SDKs and make the code compatible with Java 6 * fixed javadoc errors and added section to generate pom.xml with all builds (#551) * Updating version number to 2.0.0 * Implementing Retry logic [Reliable Channel] [STABLE Branch] (#561) * Initial commit of retry and backoff logic fixes * Fixing warnings on files I touched this round * Fix the eclipse UI from screaming about the docker Contstants * Fixed backoff logic to use existing method. Added more logging to the sender channel. * Added the partial response handler, more logging * Added gson to core. Fixed backoff manager to keep original functionality. Added extension to return the timeout values as expected before. * Added unit tests. * Fixing string typed ArrayList<> to List<> per Dhaval * Missed one * Making tests consistent. * Added javadoc comments, simplified logic for a few methods * Added exception logging per @dhaval24. Fixed formatting on touched files * Updates per last round of commits Moved the Handlers out of the concrete package to the common package to keep the same consistency. Removed a couple of unessecary methods. Added docs. * Latest fixes * Add MaxInstantRetry Added MaxInstantRetry configuration to allow for instantaneous retry on a failed transmission. * Javadoc Updates Javadoc and formatting updates * NumberFormatException fix Added null check * JavaDocs for TPM * Fixing FixedRateSampling to work in old and new version of sampling (#540) Overriding default sampling percentage when programatically specified sampling percentage by user. * upgrade to logback v1.2.3 (#565) * Reliable channel: replacing logAlways "TRACE" statements with "info" (#571) * Reliable channel: close resources in finally block. (#572) * Reliable channel: close resources in finally block. * change logging to warning when closing resources * Bugfix against retry logic (#576) * Refactor * BUGFIX Logic would never backoff After adding the instant retry amount logic to the code this line of code could cause the transmissions to not back off. * Changes requested * Fixed javadocs tags, that caused build errors while executing `javadoc` gradle task (#578) * Update Changelog * Fix link in changelog * Fix another link in changelog * Update gradle.properties * Fix customizing pom.xml in Gradle build (#582) * Fix customizing pom.xml in Gradle build * Insert license after 1. row in pom.xml * Filter artifacts relocated by shadow task from pom dependencies - match artifacts by groupId - fixes #583 * Generate a pom file "beside" the artifact jar file

Fixing FixedRateSampling to work in old and new version of sampling

e895851

anandsugumaran commented Jan 29, 2018

View reviewed changes

dhaval24 requested review from dhaval24, grlima and littleaj January 29, 2018 21:31

dhaval24 added this to the 2.0.1 milestone Jan 29, 2018

dhaval24 added the Bug label Jan 29, 2018

dhaval24 approved these changes Jan 30, 2018

View reviewed changes

littleaj approved these changes Feb 2, 2018

View reviewed changes

dhaval24 merged commit ff79610 into microsoft:master Feb 9, 2018

dhaval24 pushed a commit that referenced this pull request Feb 16, 2018

Fixing FixedRateSampling to work in old and new version of sampling (#…

621fe69

…540) Overriding default sampling percentage when programatically specified sampling percentage by user.


		samplingSupportingTelemetry.setSamplingPercentage(samplingPercentage);

		if (SamplingScoreGeneratorV2.getSamplingScore(telemetry) >= samplingPercentage) {

Fixing FixedRateSampling to work in old and new version of sampling #540

Fixing FixedRateSampling to work in old and new version of sampling #540

Uh oh!

Conversation

anandsugumaran commented Jan 29, 2018

Uh oh!

msftclas commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anandsugumaran Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

anandsugumaran Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

anandsugumaran Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

anandsugumaran left a comment

Choose a reason for hiding this comment

Uh oh!

dhaval24 commented Jan 29, 2018

Uh oh!

dhaval24 left a comment

Choose a reason for hiding this comment

Uh oh!

anandsugumaran commented Feb 2, 2018

Uh oh!

littleaj left a comment

Choose a reason for hiding this comment

Uh oh!

dhaval24 commented Feb 2, 2018

Uh oh!

anandsugumaran commented Feb 3, 2018

Uh oh!

anandsugumaran commented Feb 8, 2018

Uh oh!

dhaval24 commented Feb 8, 2018

Uh oh!

dhaval24 commented Feb 9, 2018

Uh oh!

dhaval24 commented Feb 17, 2018

Uh oh!

anandsugumaran commented Feb 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

msftclas commented Jan 29, 2018 •

edited

Loading