Skip to content

test(debugger): align DI hit-limit test with SDK MaxHits semantics - #782

Merged
liustve merged 1 commit into
aws-observability:mainfrom
liustve:fix/di-hit-limit-test-expectations
Jun 17, 2026
Merged

liustve merged 1 commit into
aws-observability:mainfrom
liustve:fix/di-hit-limit-test-expectations

Conversation

@liustve

@liustve liustve commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

The DI Flask DIFlaskHitLimitTest and the matching application target function had MaxHits semantics flipped relative to the SDK. With MaxHits=3, the test expected 2 snapshots, claiming the disable check was hit_count >= max_hits. The SDK's increment_hit_count actually uses hit_count > max_hits, so it emits 3 snapshots and disables on the 4th hit.

Why this is surfacing now

The test was passing on main by accident. BatchLogRecordProcessor's default schedule_delay_millis was 5000ms, so the snapshot emitted on the 3rd hit was still sitting in the batch buffer when the test's time.sleep(2) elapsed and read the collector. The 3rd snapshot never reached the mock collector within the test's observation window.

OpenTelemetry core 1.41.0 tightened that default to 1000ms to comply with the OTel spec, so the upcoming dependency bump to 1.42.1 / 0.63b1 (#762) now sees the 3rd snapshot consistently and the latent off-by-one assertion fails the test.

Changes

  • Update the test class docstring and both test methods so MaxHits=3 → 3 snapshots, breakpoint disabled on the 4th hit. Send a 4th request to actually probe the disabled state.
  • Update the limited_function docstring in the di-flask sample app to match.

Test plan

  • DI Debugger Contract Tests pass on this branch (currently the only test in the suite that was failing, on the nightly-dependency-updates branch)
  • No SDK behavior change — purely a test/comment alignment with current SDK semantics

@liustve
liustve requested a review from a team as a code owner June 16, 2026 19:48
@github-actions

github-actions Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @liustve's task in 1m 50s —— View job


Claude PR Review

  • Read prior reviews/comments (none)
  • Read PR diff
  • Verified SDK semantics in instrumentation_manager.py
  • Examined the PR-modified test files
  • Posted review

Verdict: LGTM

The change correctly aligns the test with the SDK's actual behavior. I verified the SDK at aws-opentelemetry-distro/src/amazon/opentelemetry/distro/debugger/instrumentation_manager.py:622-644:

state.hit_count += 1
...
if config and not config.is_permanent and state.hit_count > config.max_hits:
    state.is_disabled = True
    ...
    return False

With max_hits=3 and the strict > check, hit_count progresses 1 → 2 → 3 (all pass, three snapshots emitted), and only on the 4th hit does hit_count=4 > 3 evaluate true and disable. The new test expectations of 3 snapshots and a 4th request that does not produce a snapshot match this exactly.

Notes on robustness

  • test_breakpoint_disabled_after_hit_limit waits time.sleep(2) after the 4th request before peeking. Since the 4th hit returns from increment_hit_count before any capture happens, no snapshot is ever emitted; the sleep is only needed as a safety margin against a false-positive flush. With the new OTel core default of 1000 ms schedule_delay_millis, a 2s sleep gives ~2× headroom — fine.
  • wait_for_snapshots(min_count=3) does a >= check, so the subsequent assertEqual(initial_count, 3) is still the actual exactness gate. Good.
  • Docstrings on both the test class and limited_function now agree with the SDK (hit_count > max_hits). No stale comments left behind.

No new findings to post inline.
· Branch

@liustve liustve added the skip changelog doesn't need a CHANGELOG entry label Jun 16, 2026
srprash
srprash previously approved these changes Jun 16, 2026
@liustve
liustve enabled auto-merge (squash) June 16, 2026 19:54
@liustve
liustve force-pushed the fix/di-hit-limit-test-expectations branch from 0dcedb1 to f5f22ad Compare June 16, 2026 20:00
srprash
srprash previously approved these changes Jun 16, 2026
@aws-observability aws-observability deleted a comment from github-actions Bot Jun 16, 2026
@aws-observability aws-observability deleted a comment from github-actions Bot Jun 16, 2026
@liustve
liustve force-pushed the fix/di-hit-limit-test-expectations branch from 0523b00 to 7fc27f3 Compare June 16, 2026 22:51
@github-actions

github-actions Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @liustve's task in 1m 58s —— View job


Claude PR Review

  • Read prior reviews/comments
  • Read PR diff (gh pr diff 782)
  • Verified the only delta vs. the previously reviewed commit
  • Posted review

Verdict: LGTM — no new findings

Compared to the previously reviewed commit, the only material change in the latest push is bumping time.sleep(2) → time.sleep(5) after the 4th request in both flask_test.py and fastapi_test.py. This is purely a wider safety margin over the OTel core 1.41.0 BatchLogRecordProcessor default (schedule_delay_millis=1000) — strictly more conservative, no behavioral risk.

Everything else (SDK semantics alignment with hit_count > max_hits, the 4-request probe of the disabled state, and the matching docstrings on limited_function and the test classes) was already verified in the prior review. SDK behavior at aws-opentelemetry-distro/src/amazon/opentelemetry/distro/debugger/instrumentation_manager.py:631 confirms hit_count > max_hits, so MaxHits=3 → 3 emitted snapshots, disabled on the 4th hit, which is exactly what the new assertions encode.

Following the dedup guidance, I am not re-posting findings already covered by the earlier review on this PR.
· Branch

The SDK's increment_hit_count uses `hit_count > config.max_hits` for the
disable check (instrumentation_manager.py:631), so MaxHits=N emits N
snapshots and disables on the (N+1)th hit. The DI Flask, FastAPI, and
Django hit-limit contract tests expected N-1 snapshots, claiming the
check was `>=` in their docstrings and inline comments — neither matched
the SDK.

The tests were passing on main by accident: BatchLogRecordProcessor's
default schedule_delay_millis was 5000ms, so the snapshot emitted on the
3rd hit was still sitting in the batch buffer when the test's 2-second
sleep elapsed and read the collector. core 1.41.0 tightened the default
to 1000ms (open-telemetry/opentelemetry-python#4998), shrinking the
window enough that the 3rd snapshot now lands before the test reads.

Fix the tests (and the docstrings on the application's target functions)
to match SDK behavior: MaxHits=3 -> 3 snapshots emitted, breakpoint
disables on the 4th hit. Send a 4th request to actually probe the
disabled state, and widen the disabled-state observation window to 5s
so any spurious 4th snapshot has plenty of time to flush before we
assert the breakpoint stayed disabled.
@liustve
liustve force-pushed the fix/di-hit-limit-test-expectations branch from 7fc27f3 to d6cb052 Compare June 16, 2026 23:06
@github-actions

github-actions Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @liustve's task in 1m 29s —— View job


Claude PR Review

  • Read prior reviews/comments
  • Read PR diff (gh pr diff 782)
  • Confirmed no new commits since last review
  • Posted review

Verdict: LGTM — no new findings

The current head (d6cb052) is the same commit covered in the prior review at 22:51:37Z. There are no new commits, files, or behavioral changes to review since the last pass — this trigger fired on the same SHA.

For convenience, summary of what's been verified across the prior reviews on this PR:

  • SDK semantics at aws-opentelemetry-distro/src/amazon/opentelemetry/distro/debugger/instrumentation_manager.py:631 use hit_count > max_hits, so MaxHits=3 emits 3 snapshots and disables on the 4th hit. The new test expectations encode this exactly.
  • Both flask_test.py and fastapi_test.py send 4 requests, assert 3 snapshots after the first 3, and assert no additional snapshot after the 4th — matching the SDK.
  • django_test.py mirrors the same shape.
  • Docstrings on limited_function (di-flask, di-fastapi) and on the test classes were updated to match the SDK and don't leave any stale >= claims.
  • The time.sleep(2) → time.sleep(5) bump (the only delta in the most recent push) is a strictly more conservative observation window over OTel core 1.41.0's schedule_delay_millis=1000 default — no behavioral risk.

Per dedup guidance, not re-posting findings already covered by prior reviews.
· Branch

@liustve
liustve merged commit 7cc1ef5 into aws-observability:main Jun 17, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog doesn't need a CHANGELOG entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants