Skip to content

fix(logging): clear the selector sampler override when its handle is emptied (#7750) - #7766

Merged
Aias00 merged 1 commit into
apache:masterfrom
konlue:fix/log-selector-sampler-clear
Oct 25, 2026
Merged

Aias00 merged 1 commit into
apache:masterfrom
konlue:fix/log-selector-sampler-clear

Conversation

@konlue

@konlue konlue commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #7750.

Summary

Clearing a selector's logging configuration (empty or {} handle) left the previous entry in SELECT_API_CONFIG_MAP, and because isSampled prefers the per-selector sampler over the global one, the stale rate kept applying after the operator cleared the override.

Modifications

  • In handlerSelector, the empty/{} handle branch now removes the selector's entry from SELECT_API_CONFIG_MAP (same semantics as removeSelector), so sampling falls back to the global/default configuration.
  • Add AbstractLogPluginDataHandlerTest with a minimal concrete subclass: {} and blank handles clear the stale entry; a configured handle is still stored (regression).

Verifying this change

  • Red-green: with the removal line removed, testHandlerSelectorClearsStaleEntryOnEmptyHandle fails (the stale entry survives); with the fix, 3/3 pass.

  • Full module tests pass:

    ./mvnw -B -ntp -pl shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common test

    full module green, no regressions.

  • Checkstyle passes:

    ./mvnw -B -ntp -pl shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common checkstyle:check

    clean.

Notes

Before this change, "clearing the override" was a no-op on the data-sync path; now it restores global sampling, which is the behaviour the issue expects. Client-resource lifecycle is untouched — logCollector().close() remains confined to the plugin-enabled branch, so sampling-map cleanup and collector ownership stay decoupled.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Small change, but I checked the two things that would make it silently wrong.

Evidence

  • Key symmetry: handlerSelector stores with SELECT_API_CONFIG_MAP.put(selectorData.getId(), ...) (AbstractLogPluginDataHandler.java:139) and removeSelector removes with the same key (:146), so remove(selectorData.getId()) in the empty branch is literally the same operation as the existing delete path.
  • The fallback chain the issue depends on exists: LogCollectConfigUtils.isSampled (:106-113) prefers getSelectApiConfigMap().get(selectorData.getId()), then falls back to getPluginGlobalConfigMap().get(selectorData.getPluginId()), then true. Dropping the entry therefore restores global sampling rather than disabling logging.
  • getSelectApiConfigMap() already existed in master, so the test needs no API widening, and it clears the static maps in @AfterEach — correct hygiene for singleton state.

Non-blocking

  1. The empty-handle branch now has a side effect. handlerPlugin's disabled branch does the same kind of "undo" (logCollector().close() + map removal), so it is consistent with the class, but a short javadoc line on handlerSelector stating that an empty handle clears the override would help.
  2. If you retouch this, removeSelector(selectorData) reads better than an inline remove, and keeps the two "delete" paths from drifting apart.

CI note: pr_build is green. The only red job is e2e-case (e2e-logging-rocketmq-compose), failing on Failed to consume RocketMQ access log; the same case fails on unrelated PRs today (#7780, which only touches InstanceCheckService), so it is the known flake and not this handler.

@Aias00

Aias00 commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Approved, but the e2e checks are red on infrastructure, not on this change:

##[error]Unable to download artifact(s): Artifact not found for name: shenyu-images

The e2e-case (e2e-logging-rocketmq-compose, ...) job never got the image artifact it needs, so it failed before running any assertion — and e2e then failed as its aggregate gate. There is nothing in the log pointing at the selector sampler override.

A re-run (or a rebase, since this is 6 days old) should clear it. Ping me if it reproduces after a fresh run.

@yuluo-yx

Copy link
Copy Markdown
Member

/retest

@Aias00

Aias00 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

@konlue This PR is approved, but 2 checks are red: e2e, e2e-case (e2e-logging-rocketmq-compose, shenyu-e2e-case-logging-rocketmq, :shenyu-examples-http).

I inspected these lanes today and they fail for infrastructure reasons rather than on the diff — the integrated-test jobs die while starting their containers (Elasticsearch /_cat/health returning 000 until the job is cancelled), and the http lane also hits the known CachePluginTest.testMemoryModle / testRedisModle flake. The same jobs are red on master and on unrelated PRs right now.

Please rebase onto master and push (or ask for a re-run) so the checks execute again. If a case is still failing on a fresh run, paste the test name and I'll look properly.

@konlue
konlue force-pushed the fix/log-selector-sampler-clear branch from 4dcbd99 to e3db77a Compare October 24, 2026 15:48
@konlue

konlue commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest master to re-trigger the checks.

@Aias00
Aias00 merged commit f41d26f into apache:master Oct 25, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Clearing selector logging configuration leaves the previous sampler active

3 participants