Skip to content

feat: Expose bucketsPathsMap in BucketSelectorExtAggregationBuilder#949

Merged
eirsep merged 1 commit intoopensearch-project:mainfrom
eirsep:expose-bucketsPathsMap
Apr 29, 2026
Merged

feat: Expose bucketsPathsMap in BucketSelectorExtAggregationBuilder#949
eirsep merged 1 commit intoopensearch-project:mainfrom
eirsep:expose-bucketsPathsMap

Conversation

@eirsep
Copy link
Copy Markdown
Member

@eirsep eirsep commented Apr 29, 2026

Make bucketsPathsMap publicly accessible so that callers can read the trigger's bucket paths This is needed by the alerting plugin to translate BucketSelectorExt into a standard bucket_selector for remote trigger evaluation.

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@eirsep
feat: Expose bucketsPathsMap in BucketSelectorExtAggregationBuilder
983ce9d 
Make bucketsPathsMap publicly accessible so that callers can read the
trigger's bucket paths without reflection. This is needed by the
alerting plugin to translate BucketSelectorExt into a standard
bucket_selector for remote trigger evaluation.

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 983ce9d.

PathLineSeverityDescription
src/main/kotlin/org/opensearch/commons/alerting/aggregation/bucketselectorext/BucketSelectorExtAggregationBuilder.kt23lowVisibility of bucketsPathsMap changed from private to public (default in Kotlin). This exposes internal aggregation path mappings to external callers. The data itself (bucket path strings) is not sensitive, and the change has a plausible legitimate reason (e.g., enabling external access for serialization or testing), but it is a reduction in encapsulation worth verifying is intentional.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

API Visibility Change

Changing bucketsPathsMap from private to val (public) is a breaking change in terms of encapsulation. Verify that exposing this field does not allow unintended mutation of the map contents (e.g., if the underlying implementation is a mutable map cast to Map<String, String>, callers could cast it back to MutableMap). Ensure the map is truly immutable or defensively copied before being stored.

 
val bucketsPathsMap: Map<String, String>

@eirsep eirsep merged commit 2f9a97c into opensearch-project:main Apr 29, 2026
11 checks passed
eirsep added a commit to eirsep/alerting that referenced this pull request Apr 29, 2026
@eirsep
fix: Address PR review comments — remove reflection, add runtime vali…
31f8028 
…dation

- BucketSelectorQueryBuilder: use public bucketsPathsMap getter from
  common-utils instead of reflection (opensearch-project/common-utils#949)
- BucketKeyFilter: use IncludeExclude.convertToStringFilter() instead
  of reflection on private fields
- TriggerService: replace blanket @Suppress(UNCHECKED_CAST) with
  runtime type checks using require()
- BucketLevelMonitorRunner: add 1-trigger validation at execution time
  to cover the _execute API path (in addition to create/update validation)

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
eirsep added a commit to opensearch-project/alerting that referenced this pull request Apr 29, 2026
@eirsep
Bucket level multi-tenant trigger (#2098)
a1e869c 
* feat(bucket-trigger): Add bucket-level trigger evaluation using standard bucket_selector

Replace the custom BucketSelectorExt aggregation with standard
bucket_selector for bucket-level trigger evaluation when
multi_tenant_trigger_eval_enabled is true. This enables trigger
evaluation on user clusters that do not have the alerting plugin
installed.

**When the flag is on, bucket-level monitors are limited to 1 trigger.**
The standard bucket_selector is injected directly into the monitor's
search query so a single search call performs both data collection and
trigger evaluation. Include/exclude filtering from BucketSelectorExtFilter
is replicated post-response by BucketKeyFilter.

The existing BucketSelectorExt path is completely unchanged when the
flag is off (the default).

New files:
- BucketSelectorQueryBuilder: injects standard bucket_selector sub-agg
- BucketKeyFilter: post-response include/exclude bucket filtering
- TriggerService.runBucketLevelTriggerFromFilteredResponse(): reads
  remaining buckets from filtered response

Modified files:
- BucketLevelMonitorRunner: flag-gated single-query path
- InputService: useStandardBucketSelector parameter
- TransportIndexMonitorAction: 1-trigger validation for bucket-level
  monitors when flag is on
- AggregationQueryRewriter: skipBucketSelectorInjection parameter

Integration tests:
- RemoteBucketLevelTriggerIT: 13 tests covering composite/terms agg,
  alert lifecycle, dry run, nested parent path, include/exclude filter,
  validation of 1-trigger limit, Painless script in query
- RemoteBucketLevelTriggerRegressionIT: 5 tests verifying flag=false
  preserves existing BucketSelectorExt behavior

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>

* fix: Remove dead code and clean up PR

- Remove collectInputResultsForTrigger() from InputService (dead code
  from per-trigger query approach)
- Remove skipAllBucketSelectorInjection parameter from collectInputResults
  and getSearchRequest (no longer used)
- Collapse identical triggerCtx if/else branches in BucketLevelMonitorRunner
- Restore .gitignore to original state

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>

* fix: Address PR review comments — remove reflection, add runtime validation

- BucketSelectorQueryBuilder: use public bucketsPathsMap getter from
  common-utils instead of reflection (opensearch-project/common-utils#949)
- BucketKeyFilter: use IncludeExclude.convertToStringFilter() instead
  of reflection on private fields
- TriggerService: replace blanket @Suppress(UNCHECKED_CAST) with
  runtime type checks using require()
- BucketLevelMonitorRunner: add 1-trigger validation at execution time
  to cover the _execute API path (in addition to create/update validation)

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>

---------

Signed-off-by: Surya Sashank Nistala <snistala@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@riysaxen-amzn riysaxen-amzn riysaxen-amzn approved these changes

@vikhy-aws vikhy-aws vikhy-aws approved these changes

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

@bowenlan-amzn bowenlan-amzn Awaiting requested review from bowenlan-amzn bowenlan-amzn is a code owner

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

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

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

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

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

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

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

@nishtham-amazon nishtham-amazon Awaiting requested review from nishtham-amazon nishtham-amazon is a code owner

@KashKondaka KashKondaka Awaiting requested review from KashKondaka KashKondaka 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.

3 participants

@eirsep @riysaxen-amzn @vikhy-aws