Expose wrapped scorer in ProfileScorer via getWrappedScorer method#20549
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR adds a public accessor method Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ Gradle check result for 91ad9df: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
martin-gaievski
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
|
Failing test is unrelated to this change, found using OpenSearch Gradle Check Metrics Dashboard that this is usual suspect for flaky tests. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 11: The changelog entry for exposing the wrapped scorer should be moved
from the "Changed" section to the "Added" section because it introduces a new
public API; locate the line mentioning "Expose wrapped scorer in ProfileScorer
via getWrappedScorer" (reference symbol: getWrappedScorer and ProfileScorer) in
CHANGELOG.md and cut/paste that bullet into the "Added" section, keeping the
copy and issue reference intact and removing it from the "Changed" section.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdserver/src/main/java/org/opensearch/search/profile/query/ProfileScorer.javaserver/src/test/java/org/opensearch/search/profile/query/ProfileScorerTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:40:27.167Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.
Applied to files:
server/src/main/java/org/opensearch/search/profile/query/ProfileScorer.java
🔇 Additional comments (2)
server/src/test/java/org/opensearch/search/profile/query/ProfileScorerTests.java (1)
105-114: Good coverage for the new accessor.server/src/main/java/org/opensearch/search/profile/query/ProfileScorer.java (1)
68-86: No changes needed—the design is correct.ProfileScorer is intentionally package-private and marked
@opensearch.internal. External plugins access thegetWrappedScorer()method polymorphically through the publicScorerinterface, not by directly referencing theProfileScorerclass. This is a standard pattern in Java instrumentation and requires no utility class or visibility modification. The implementation properly supports the documented use case for plugins like neural-search.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
❌ Gradle check result for ba22d8f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for ba22d8f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for ba22d8f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for ba22d8f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
jainankitk
left a comment
jainankitk
left a comment
There was a problem hiding this comment.
While the solution works for addressing the inconsistency across collectors/scorers, I am wondering if there is a better way since HybridBulkScorer will not be profiled even when the profiling option is set to true, right?
Thanks for the thoughtful review. You raise a valid technical point about BulkScorer-level profiling. With this change, hybrid queries are now fully profiled at the query/weight level. Here is summary of what is in profiler output based on the POC I built using changes from this PR HybridQuery: time_in_nanos: 12,444,791
├── breakdown.create_weight: 4,008,459
├── breakdown.build_scorer: 8,321,958
├── breakdown.next_doc: 11,249
│
├── children[0]: BooleanQuery (match) → time_in_nanos: 12,413,459
│ ├── children: TermQuery "name:cookies" → time_in_nanos: 11,457,584
│ ├── children: TermQuery "name:kitchen" → time_in_nanos: 8,749,625
│ └── children: TermQuery "name:aroma" → time_in_nanos: 8,671,626
│
├── children[1]: IndexOrDocValuesQuery (range) → time_in_nanos: 8,576,000
└── children[2]: LuceneEngineKnnVectorQuery (knn) → time_in_nanos: 8,562,458detailed response is in neural-search PR opensearch-project/neural-search#1255 (comment). this provides:
Looking at OpenSearch core, there is no
But
HybridBulkScorer's internal operations (window scoring, doc ID iteration) are indirectly captured in the parent HybridQuery's
However, this level of detail is rarely actionable for users - what they need to know is "which sub-query is slow?", which the current profiling fully answers. Adding BulkScorer profiling (in the way I see it) would require:
This could be a separate enhancement PR, but is beyond the scope of fixing the original issue opensearch-project/neural-search#1255 which was specifically about getting profiling to work at all for hybrid query |
|
This time there is bunch of |
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
martin-gaievski
force-pushed
the
profile-scorer-get-wrapped-scorer
branch
from
ba22d8f to
07ae015
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20549 +/- ##
============================================
+ Coverage 73.25% 73.28% +0.02%
- Complexity 72103 72126 +23
============================================
Files 5798 5798
Lines 329732 329733 +1
Branches 47519 47519
============================================
+ Hits 241554 241641 +87
+ Misses 68805 68720 -85
+ Partials 19373 19372 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…pensearch-project#20549) Signed-off-by: Martin Gaievski <gaievski@amazon.com>
…pensearch-project#20549) Signed-off-by: Martin Gaievski <gaievski@amazon.com>
2 participants
Description
This change exposes the wrapped scorer in
ProfileScorervia a newgetWrappedScorer()method.Background: When profiling is enabled, OpenSearch wraps scorers with
ProfileScorerto collect timing metrics. However, plugin queries (like neural-search'sHybridQuery) may extend the standard LuceneScorerAPI with custom methods. Currently, plugins cannot access these custom methods when profiling is enabled because the wrapped scorer is stored in a private field with no accessor.Solution: Add a
getWrappedScorer()method that returns the wrapped scorer, following the same pattern already established in the codebase:ProfileCollector.getDelegate()(same profiling package)ProfilingAggregator.unwrapAggregator()(aggregation profiling)Use case: neural-search's
HybridQueryneeds to calldocIdStream()on itsHybridBulkScorer, which is not part of the standard LuceneScorerAPI.Related Issues
#20548
Check List
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.