Fix sampling log probe with capture expressions#11571
Conversation
A log probe with CaptureExpressions has captureSnapshot=false but this is not a log template so the sampling must be 1/s like a regular snapshot
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
jpbempel
added
type: bug
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09f04eb975
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!hasCondition()) { | ||
| // we are sampling here to avoid creating CapturedContext when the sampling result is negative | ||
| return ProbeRateLimiter.tryProbe(sampler, isCaptureSnapshot()); | ||
| return ProbeRateLimiter.tryProbe(sampler, isFullSnapshot()); |
There was a problem hiding this comment.
For a capture-expression log probe without explicit sampling, this now selects the snapshot global limiter, but initSamplers() still builds sampler with DEFAULT_LOG_RATE whenever captureSnapshot(false) is set. In that scenario the per-probe limiter remains 5000/s even though capture expressions are committed as full snapshots, so one probe can overproduce until the global snapshot budget is exhausted; base the default sampler rate on isFullSnapshot() as well.
Useful? React with 👍 / 👎.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Debugger benchmarksParameters
See matching parameters
SummaryFound 5 performance improvements and 0 performance regressions! Performance is the same for 4 metrics, 6 unstable metrics.
See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (371.462 µs) : 308, 435
. : milestone, 371,
basic (309.385 µs) : 301, 317
. : milestone, 309,
loop (9.772 ms) : 9630, 9914
. : milestone, 9772,
section candidate
noprobe (369.424 µs) : 317, 422
. : milestone, 369,
basic (311.772 µs) : 303, 320
. : milestone, 312,
loop (9.016 ms) : 9008, 9023
. : milestone, 9016,
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 5920116106124784514 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
What Does This Do
A log probe with CaptureExpressions has captureSnapshot=false but this is not a log template so the sampling must be 1/s like a regular snapshot
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [DEBUG-5730]