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.

Sign up for GitHub

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

Jump to bottom

Rate limited sampling #2456

Merged
merged 16 commits into from
Aug 29, 2022
Merged

Rate limited sampling #2456

merged 16 commits into from
Aug 29, 2022

Conversation

trask
Copy link
Member

@trask trask commented Aug 21, 2022 •
edited
Loading

Not ready for review, design is still in flux.

In this PR:

  • Introduces rate limited sampling
  • Splits sampling overrides into requests/dependencies/logs/exceptions
  • Changes default sampling behavior from fixed rate sampling of 100% to rate limited sampling of up to 5 requests per second
  • Changes default fixed rate sampling behavior to not apply to "standalone" telemetry (typically startup logs/dependencies)
  • Changes sampling overrides to not apply to "standalone telemetry" (typically startup logs/dependencies)

Resolves #2416
Resolves #1974 (in conjunction with PR #2443)
Resolves #1973

@trask trask force-pushed the rate-limited-sampling branch from 876ce68 to 7ab8758 Compare August 21, 2022 03:56
@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2022

This pull request introduces 1 alert when merging 7ab8758 into cfb9ca6 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask trask force-pushed the rate-limited-sampling branch 2 times, most recently from efefb66 to e7d1bde Compare August 21, 2022 16:55
@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2022

This pull request introduces 1 alert when merging e7d1bde into cfb9ca6 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask trask force-pushed the rate-limited-sampling branch from e7d1bde to bd15ec7 Compare August 21, 2022 17:18
@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2022

This pull request introduces 1 alert when merging bd15ec7 into cfb9ca6 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask trask mentioned this pull request Aug 22, 2022
Closed
Base automatically changed from update-otel to main August 23, 2022 22:10
@trask trask force-pushed the rate-limited-sampling branch 2 times, most recently from 69e532c to dd17b80 Compare August 24, 2022 18:55
@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2022

This pull request introduces 1 alert when merging e47aeaa into b13a24f - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask trask force-pushed the rate-limited-sampling branch 2 times, most recently from 6b6cf49 to 01c823d Compare August 24, 2022 20:13
trask
trask commented Aug 24, 2022
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2022

This pull request introduces 1 alert when merging 01c823d into b13a24f - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask trask force-pushed the rate-limited-sampling branch from 01c823d to 8bb4b5b Compare August 24, 2022 20:36
@trask trask force-pushed the rate-limited-sampling branch from fb10f70 to 9deab0e Compare August 24, 2022 21:04
@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2022

This pull request introduces 1 alert when merging 9deab0e into b13a24f - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask trask marked this pull request as ready for review August 24, 2022 21:25
@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2022

This pull request introduces 1 alert when merging d543f28 into 2170998 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask
Copy link
Member Author

trask commented Aug 26, 2022

two follow-ups for after this PR:

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2022

This pull request introduces 1 alert when merging 4758211 into 038342c - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2022

This pull request introduces 1 alert when merging 3641cbe into 038342c - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2022

This pull request introduces 1 alert when merging e0c5098 into 86cba45 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2022

This pull request introduces 1 alert when merging 0b2afed into 86cba45 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2022

This pull request introduces 1 alert when merging 48b3d8e into 86cba45 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2022

This pull request introduces 1 alert when merging 1c24cd3 into 86cba45 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

@trask
Copy link
Member Author

trask commented Aug 29, 2022

two follow-ups for after this PR:

this proved a bit difficult, and there should be upstream log sampling at some point, so a good time to revisit this if not sooner

created a work item for this one

@trask
Copy link
Member Author

trask commented Aug 29, 2022

This pull request introduces 1 alert when merging 1c24cd3 into 86cba45 - view on LGTM.com

new alerts:

  • 1 for Cross-site scripting

ignore this, I fixed it (and others) in #2468

heyams
heyams reviewed Aug 29, 2022
View reviewed changes
...c/main/java/com/microsoft/applicationinsights/agent/internal/sampling/AiOverrideSampler.java
Comment on lines +70 to +71
boolean standaloneTelemetry = !isRequest && !parentSpanContext.isValid();
Sampler override = samplingOverrides.getOverride(standaloneTelemetry, attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
boolean standaloneTelemetry = !isRequest && !parentSpanContext.isValid();
Sampler override = samplingOverrides.getOverride(standaloneTelemetry, attributes);
boolean isStandaloneTelemetry = !isRequest && !parentSpanContext.isValid();
Sampler override = samplingOverrides.getOverride(isStandaloneTelemetry , attributes);

Copy link
Member Author

@trask trask Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

conventionally in Java boolean fields are not prefixed with is, while their getters are

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed this change to the next PR to avoid conflicts

.../src/smokeTest/java/com/microsoft/applicationinsights/smoketest/RateLimitedSamplingTest.java

// average response time of 10 ms, times 1000 requests, equals 10 seconds
// so ideally with rate of 0.5 requests per second we would get 5 requests
assertThat(rdEnvelopes.size()).isLessThan(20);
Copy link
Contributor

Choose a reason for hiding this comment

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

why 20? expected roughly around ~5 requests?

Copy link
Member Author

Choose a reason for hiding this comment

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

~5 requests is correct, it's somewhat random though and don't want to introduce sporadic test failures in CI

@trask trask merged commit fd57874 into main Aug 29, 2022
@trask trask deleted the rate-limited-sampling branch August 29, 2022 20:03
@asos-jitu
Copy link

Are we planning to update the details in MS documentation around sampling? Reading through the comments and changes this looks to me as an implicit behaviour than a configurable one.
https://docs.microsoft.com/en-us/azure/azure-monitor/app/sampling

@trask
Copy link
Member Author

trask commented Aug 31, 2022

hi @asos-jitu! yes, the documentation has been updated at https://docs.microsoft.com/en-us/azure/azure-monitor/app/java-standalone-config#sampling, and once GA is released we will add mention of the behavior changes to https://docs.microsoft.com/en-us/azure/azure-monitor/app/java-in-process-agent#download-the-jar-file

@asos-jitu
Copy link

Hi @trask , does rate-limiting represents adaptive sampling by definition? How the volume-based adaptation matches with adaptive sampling?

https://docs.microsoft.com/en-us/azure/azure-monitor/app/sampling#adaptive-sampling

@trask
Copy link
Member Author

trask commented Sep 2, 2022

Hi @trask , does rate-limiting represents adaptive sampling by definition?

Yes

How the volume-based adaptation matches with adaptive sampling?

I'm not sure I follow, can you clarify this question?

@asos-jitu
Copy link

Hi @trask, how does the following config work and be adjusted as per volume? Do we have any tests which proves the behaviour?
"sampling": {
"percentage": 50.00,
"limitPerSecond": 5
}

@asos-jitu
Copy link

Hi @trask, as per release notes rate limiting is for request types. Does that mean the other dependencies are not supported?

Both percentage-based and rate-limited sampling now only apply to requests, and to telemetry that is captured in the context of a request. If you do want to sample "standalone" telemetry (e.g. startup logs), you can use sampling overrides, e.g.
{
"preview": {
"sampling": {
"overrides": [
{
"telemetryType": "dependency",
"includingStandaloneTelemetry": true,
"percentage": 10
}
]
}
}
}

@trask
Copy link
Member Author

trask commented Sep 7, 2022

Hi @trask, how does the following config work and be adjusted as per volume? Do we have any tests which proves the behaviour? "sampling": { "percentage": 50.00, "limitPerSecond": 5 }

you pick one or the other, either fixed percentage, or limitPerSecond

for tests, see the Sampling* smoke tests under https://github.com/microsoft/ApplicationInsights-Java/tree/main/smoke-tests/apps

@asos-jitu
Copy link

Hi @trask, given limitPerSecond caters to requests only and percentage caters to all other types including dependencies/exceptions etc. How one would sample both as that's a usual pattern?

@trask
Copy link
Member Author

trask commented Sep 9, 2022

limitPerSecond applies to requests, but then all other telemetry that is captured inside of requests follows suit (sampled if and only if the request is sampled)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heyams heyams heyams approved these changes

@jeanbisutti jeanbisutti Awaiting requested review from jeanbisutti jeanbisutti is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@trask @asos-jitu @heyams