Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Search Pipelines] Add stats for search pipelines #8053

Merged
merged 4 commits into from Jun 28, 2023

Conversation

msfroh
Copy link
Collaborator

@msfroh msfroh commented Jun 14, 2023

Description

This adds statistics on executions and time spent on search pipeline operations, similar to the stats that are available for ingest pipelines.

Related Issues

Resolves #6723

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #8053 (159e568) into main (1cf9c5c) will increase coverage by 0.03%.
The diff coverage is 87.93%.

@@             Coverage Diff              @@
##               main    #8053      +/-   ##
============================================
+ Coverage     70.89%   70.92%   +0.03%     
- Complexity    56639    56656      +17     
============================================
  Files          4722     4725       +3     
  Lines        267674   267987     +313     
  Branches      39232    39267      +35     
============================================
+ Hits         189757   190063     +306     
+ Misses        61937    61864      -73     
- Partials      15980    16060      +80     
Impacted Files Coverage Δ
...min/cluster/stats/TransportClusterStatsAction.java 69.56% <ø> (ø)
...src/main/java/org/opensearch/node/NodeService.java 73.49% <0.00%> (-0.90%) ⬇️
...rch/action/admin/cluster/node/stats/NodeStats.java 50.54% <33.33%> (-0.90%) ⬇️
.../action/admin/cluster/stats/ClusterStatsNodes.java 55.00% <55.55%> (+5.83%) ⬆️
.../org/opensearch/common/metrics/OperationStats.java 78.78% <78.78%> (ø)
.../java/org/opensearch/search/pipeline/Pipeline.java 87.83% <85.96%> (-1.82%) ⬇️
...pensearch/search/pipeline/SearchPipelineStats.java 89.44% <89.44%> (ø)
...pensearch/search/pipeline/PipelineWithMetrics.java 90.90% <90.90%> (ø)
...main/java/org/opensearch/ingest/IngestService.java 83.37% <93.75%> (-1.16%) ⬇️
...on/admin/cluster/node/stats/NodesStatsRequest.java 93.33% <100.00%> (+0.09%) ⬆️
... and 8 more

... and 481 files with indirect coverage changes

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockIsRemovedWhenAnyNodesNotExceedHighWatermarkWithAutoReleaseEnabled

@msfroh
Copy link
Collaborator Author

msfroh commented Jun 23, 2023

@saratvemulapalli -- Addressed your suggestions.

Windows precommit failed, but that seems to be a "Windows build server" thing and not a "this PR" thing.

@saratvemulapalli
Copy link
Member

@saratvemulapalli -- Addressed your suggestions.

Windows precommit failed, but that seems to be a "Windows build server" thing and not a "this PR" thing.

Thanks @msfroh. I've triggered a re-run of window precommit. I haven't seen failures just in Windows platform.
If the failure persists we can dig in.

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Thanks @msfroh for taking care of the feedback.
LGTM!

@msfroh
Copy link
Collaborator Author

msfroh commented Jun 26, 2023

Thanks, @saratvemulapalli!

I resolved the open conversations, so it looks like there are no immediate merge blockers.

@msfroh
Copy link
Collaborator Author

msfroh commented Jun 26, 2023

Oh, the "Some checks were not successful" note seems to be there because the Mac OS precommit was canceled when the Windows precommit failed.

If someone can retry the Mac OS precommit, I think that should turn green.

@dblock dblock merged commit 46c9a21 into opensearch-project:main Jun 28, 2023
10 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-8053-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 46c9a211b6b9490f6a7ac9425e946986cd51bed2
# Push it to GitHub
git push --set-upstream origin backport/backport-8053-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8053-to-2.x.

@dblock
Copy link
Member

dblock commented Jun 28, 2023

@msfroh needs manual backport if you want this in 2.x

@mingshl
Copy link
Contributor

mingshl commented Jun 29, 2023

I will help with the backport to 2.x

mingshl pushed a commit to mingshl/OpenSearch-Mingshl that referenced this pull request Jun 30, 2023
…#8053)

* [Search Pipelines] Add stats for search pipelines

This adds statistics on executions and time spent on search pipeline
operations, similar to the stats that are available for ingest
pipelines.

Signed-off-by: Michael Froh <froh@amazon.com>

* Compare parsed JSON structure, not exact JSON string

As @lukas-vlcek pointed out, asserting equality with an exact JSON
string is sensitive to formatting, which makes the test brittle.

Instead, we can parse the expected JSON and compare as Maps.

Signed-off-by: Michael Froh <froh@amazon.com>

* Refactor to common stats/metrics classes

Search pipelines and ingest pipelines had identical functionality for
tracking metrics around operations and converting those to immutable
"stats" objects.

That approach isn't even really specific to pipelines, but can be used
to track metrics on any repeated operation, so I moved that common
logic to the common.metrics package.

Signed-off-by: Michael Froh <froh@amazon.com>

* Split pipeline metrics tracking into its own class

Thanks @saratvemulapalli for the suggestion! This lets the Pipeline
class focus on transforming requests / responses, while the subclass
focuses on tracking and managing metrics.

Signed-off-by: Michael Froh <froh@amazon.com>

---------

Signed-off-by: Michael Froh <froh@amazon.com>
(cherry picked from commit 46c9a21)
mingshl pushed a commit to mingshl/OpenSearch-Mingshl that referenced this pull request Jul 5, 2023
…#8053)

* [Search Pipelines] Add stats for search pipelines

This adds statistics on executions and time spent on search pipeline
operations, similar to the stats that are available for ingest
pipelines.

Signed-off-by: Michael Froh <froh@amazon.com>

* Compare parsed JSON structure, not exact JSON string

As @lukas-vlcek pointed out, asserting equality with an exact JSON
string is sensitive to formatting, which makes the test brittle.

Instead, we can parse the expected JSON and compare as Maps.

Signed-off-by: Michael Froh <froh@amazon.com>

* Refactor to common stats/metrics classes

Search pipelines and ingest pipelines had identical functionality for
tracking metrics around operations and converting those to immutable
"stats" objects.

That approach isn't even really specific to pipelines, but can be used
to track metrics on any repeated operation, so I moved that common
logic to the common.metrics package.

Signed-off-by: Michael Froh <froh@amazon.com>

* Split pipeline metrics tracking into its own class

Thanks @saratvemulapalli for the suggestion! This lets the Pipeline
class focus on transforming requests / responses, while the subclass
focuses on tracking and managing metrics.

Signed-off-by: Michael Froh <froh@amazon.com>

---------

Signed-off-by: Michael Froh <froh@amazon.com>
(cherry picked from commit 46c9a21)
tlfeng pushed a commit that referenced this pull request Jul 5, 2023
backport commit 46c9a21 to `2.x` branch.
* [Search Pipelines] Add stats for search pipelines (#8053)

This adds statistics on executions and time spent on search pipeline
operations, similar to the stats that are available for ingest
pipelines.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Co-authored-by: Michael Froh <froh@amazon.com>
tlfeng pushed a commit that referenced this pull request Jul 5, 2023
Update the version of BWC test from 3.0 to 2.9 for search pipeline statistic after the PR #8053 / commit 46c9a21 backported to 2.x branch.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
vikasvb90 pushed a commit to raghuvanshraj/OpenSearch that referenced this pull request Jul 12, 2023
Update the version of BWC test from 3.0 to 2.9 for search pipeline statistic after the PR opensearch-project#8053 / commit opensearch-project@46c9a21 backported to 2.x branch.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
raghuvanshraj pushed a commit to raghuvanshraj/OpenSearch that referenced this pull request Jul 12, 2023
Update the version of BWC test from 3.0 to 2.9 for search pipeline statistic after the PR opensearch-project#8053 / commit opensearch-project@46c9a21 backported to 2.x branch.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
buddharajusahil pushed a commit to buddharajusahil/OpenSearch that referenced this pull request Jul 18, 2023
Update the version of BWC test from 3.0 to 2.9 for search pipeline statistic after the PR opensearch-project#8053 / commit opensearch-project@46c9a21 backported to 2.x branch.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Signed-off-by: sahil buddharaju <sahilbud@amazon.com>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
…#8053)

* [Search Pipelines] Add stats for search pipelines

This adds statistics on executions and time spent on search pipeline
operations, similar to the stats that are available for ingest
pipelines.

Signed-off-by: Michael Froh <froh@amazon.com>

* Compare parsed JSON structure, not exact JSON string

As @lukas-vlcek pointed out, asserting equality with an exact JSON
string is sensitive to formatting, which makes the test brittle.

Instead, we can parse the expected JSON and compare as Maps.

Signed-off-by: Michael Froh <froh@amazon.com>

* Refactor to common stats/metrics classes

Search pipelines and ingest pipelines had identical functionality for
tracking metrics around operations and converting those to immutable
"stats" objects.

That approach isn't even really specific to pipelines, but can be used
to track metrics on any repeated operation, so I moved that common
logic to the common.metrics package.

Signed-off-by: Michael Froh <froh@amazon.com>

* Split pipeline metrics tracking into its own class

Thanks @saratvemulapalli for the suggestion! This lets the Pipeline
class focus on transforming requests / responses, while the subclass
focuses on tracking and managing metrics.

Signed-off-by: Michael Froh <froh@amazon.com>

---------

Signed-off-by: Michael Froh <froh@amazon.com>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
Update the version of BWC test from 3.0 to 2.9 for search pipeline statistic after the PR opensearch-project#8053 / commit opensearch-project@46c9a21 backported to 2.x branch.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
@lukas-vlcek
Copy link
Contributor

Hi,

I wanted to friendly point out that IMO PRs like this should have had the breaking label.

I am maintaining external plugin (specifically the Prometheus exporter plugin) and as you can imagine renaming some of the methods to get operational stats about Ingest operations had impact (as the plugin needs to call them).

Specifically:

nodeIngestStats.getIngestCount()        ->  getCount()
nodeIngestStats.getIngestFailedCount()  ->  getFailedCount()
nodeIngestStats.getIngestCurrent()      ->  getCurrent()
nodeIngestStats.getIngestTimeInMillis() ->  getTotalTimeInMillis()

I totally get it that given current state of API isolation it is possible that any internal renaming is possibly a "breaking" change but hey ... it is. I do not know if it makes sense to establish a new rule about how the "breaking" label is applied but the fact is that change like this can cause compilation error somewhere else.

Lukáš

@msfroh
Copy link
Collaborator Author

msfroh commented Aug 1, 2023

I totally get it that given current state of API isolation it is possible that any internal renaming is possibly a "breaking" change but hey ... it is. I do not know if it makes sense to establish a new rule about how the "breaking" label is applied but the fact is that change like this can cause compilation error somewhere else.

Oh, shoot. I didn't expect that.

I'm not sure what the best option is there. I'll aim to add the "breaking" label if I rename an existing method in future, at least.

@lukas-vlcek
Copy link
Contributor

@msfroh Do not feel sorry for this.
With 2.9.0 there was another breaking change that impacted me, see #7508 (comment)

I also do not know what is the solution here. We could introduce something like "internal_API_change" label which will not go to the change log but can be used on GitHub to filter tickets but it would have to be applied consistently and there is no way how to achieve this (except by increasing the work load).

lukas-vlcek added a commit to lukas-vlcek/prometheus-exporter-plugin-for-opensearch that referenced this pull request Aug 1, 2023
The methods to get operation stats for Ingest operations were renamed.

See the following ticket for more details:
<opensearch-project/OpenSearch#8053>

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
lukas-vlcek added a commit to Aiven-Open/prometheus-exporter-plugin-for-opensearch that referenced this pull request Aug 1, 2023
The methods to get operation stats for Ingest operations were renamed.

See the following ticket for more details:
<opensearch-project/OpenSearch#8053>

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…#8053)

* [Search Pipelines] Add stats for search pipelines

This adds statistics on executions and time spent on search pipeline
operations, similar to the stats that are available for ingest
pipelines.

Signed-off-by: Michael Froh <froh@amazon.com>

* Compare parsed JSON structure, not exact JSON string

As @lukas-vlcek pointed out, asserting equality with an exact JSON
string is sensitive to formatting, which makes the test brittle.

Instead, we can parse the expected JSON and compare as Maps.

Signed-off-by: Michael Froh <froh@amazon.com>

* Refactor to common stats/metrics classes

Search pipelines and ingest pipelines had identical functionality for
tracking metrics around operations and converting those to immutable
"stats" objects.

That approach isn't even really specific to pipelines, but can be used
to track metrics on any repeated operation, so I moved that common
logic to the common.metrics package.

Signed-off-by: Michael Froh <froh@amazon.com>

* Split pipeline metrics tracking into its own class

Thanks @saratvemulapalli for the suggestion! This lets the Pipeline
class focus on transforming requests / responses, while the subclass
focuses on tracking and managing metrics.

Signed-off-by: Michael Froh <froh@amazon.com>

---------

Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
Update the version of BWC test from 3.0 to 2.9 for search pipeline statistic after the PR opensearch-project#8053 / commit opensearch-project@46c9a21 backported to 2.x branch.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch Search Search query, autocomplete ...etc v2.9.0 'Issues and PRs related to version v2.9.0' v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search pipeline metrics (at least on par w/ ingestion pipeline metrics).
5 participants