Skip to content

Better sampling override behavior #4014

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
Merged
merged 14 commits into from
Feb 24, 2025
Merged

Better sampling override behavior #4014

merged 14 commits into from
Feb 24, 2025

Conversation

trask
Copy link
Member

@trask trask commented Jan 9, 2025 •
edited
Loading

(icm 577578458)

Currently, this configuration:

      {
        "telemetryType": "request",
        "attributes": [
          {
            "key": "http.url",
            "value": ".*/health-check",
            "matchType": "regexp"
          }
        ],
        "percentage": 0,
        "id": "filter out health check"
      },
      {
        "telemetryType": "dependency",
        "attributes": [
          {
            "key": "db.statement",
            "value": "select count(*) from abc",
            "matchType": "strict"
          }
        ],
        "percentage": 50,
        "id": "filter out noisy jdbc"
      }

has the unexpected effect of capturing 50% of select count(*) from abc calls, even when they occur within the excluded health check.

this PR updates the behavior so that when a sampling override percentage is <100 and a parent dropped, then the span (with override percentage < 100) will also be dropped

@trask trask force-pushed the sampling-override-behavior branch 2 times, most recently from 59be9a0 to 97276ab Compare January 14, 2025 03:13
@trask trask changed the title More expected sampling override behavior Better sampling override behavior Jan 14, 2025
@trask trask force-pushed the sampling-override-behavior branch from 97276ab to 90d4ada Compare January 22, 2025 03:14
@trask trask force-pushed the sampling-override-behavior branch from 90d4ada to 11b4832 Compare January 22, 2025 03:15
@trask trask marked this pull request as ready for review January 22, 2025 15:31
jeanbisutti
jeanbisutti reviewed Feb 19, 2025
View reviewed changes
Copy link
Member

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

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

Could you please make the PR title more specific? Perhaps something like "Propagate the request sampling percentage"?

jeanbisutti
jeanbisutti reviewed Feb 20, 2025
View reviewed changes
...rc/main/java/com/microsoft/applicationinsights/agent/internal/exporter/AgentLogExporter.java
if (!hasSamplingOverride
&& spanContext.isValid()
&& !spanContext.getTraceFlags().isSampled()) {
// if there is no sampling override, and the log is part of an unsampled trace,
Copy link
Member

@jeanbisutti jeanbisutti Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Because the scenarios are not super simple, the code comments could maybe try to refer to tests checking this scenario

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 totally agree that the scenarios are complex, this particular comment though I think it pretty clear (relatively, at least, compared to some of the other code in this PR)? maybe you had in mind other places?

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion was to refer to tests in the comments, in addition to the current comment explanations.

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'd prefer to just improve the comments if they are unclear. Or if you make specific suggestions linking these to tests I'm happy to hit accept.

@trask trask merged commit aa2d6f3 into main Feb 24, 2025
91 checks passed
@trask trask deleted the sampling-override-behavior branch February 24, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanbisutti jeanbisutti jeanbisutti approved these changes

@heyams heyams Awaiting requested review from heyams

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

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

@rajkumar-rangaraj rajkumar-rangaraj Awaiting requested review from rajkumar-rangaraj rajkumar-rangaraj is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trask @jeanbisutti