Cluster config api return verbose PA status

[dzane17]

Signed-off-by: David Zane [email protected]

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
Fixes #322

When checking cluster state of Performance Analyzer, the responses are bit-encoded numeric values, which don't provide much meaningful insight.

Current cluster/config output:

GET localhost:9200/_plugins/_performanceanalyzer/cluster/config
{"batchMetricsRetentionPeriodMinutes": 7, "currentPerformanceAnalyzerClusterState": 19, "shardsPerCollection": 0}

Describe the solution you are proposing
Translate bitmap to Map<String, Boolean> output when ?verbose query param is provided.

New cluster/config output:

% curl -XGET localhost:9200/_plugins/_performanceanalyzer/cluster/config?verbose
{"currentPerformanceAnalyzerClusterState":{"PerformanceAnalyzerEnabled":true,"RcaEnabled":true,"LoggingEnabled":false,"BatchMetricsEnabled":false,"ThreadContentionMonitoringEnabled":false},"shardsPerCollection":0,"batchMetricsRetentionPeriodMinutes":7}

Also works for these GET/POST APIs:

/cluster/config?verbose
/rca/cluster/config?verbose
/logging/cluster/config?verbose
/batch/cluster/config?verbose
/threadContentionMonitoring/cluster/config?verbose

Describe alternatives you've considered
I have kept bitmap implementation instead of converting to some map object because of the provided Setting construct. We are using Setting.intSetting(). There is no map option (int, float, long, boolean... available).

Also, int representation will help a bit with memory.

Additional context
Add any other context or screenshots about the feature request here.

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

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.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.88%. Comparing base (066cef8) to head (a3efbd5).
Report is 88 commits behind head on main.

Files with missing lines	Patch %	Lines
...config/PerformanceAnalyzerClusterConfigAction.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #342      +/-   ##
============================================
+ Coverage     70.84%   70.88%   +0.04%     
- Complexity      373      377       +4     
============================================
  Files            44       44              
  Lines          2583     2597      +14     
  Branches        174      176       +2     
============================================
+ Hits           1830     1841      +11     
- Misses          641      647       +6     
+ Partials        112      109       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sgup432]

As this is a breaking change(as api response is essentially changing), I think we should instead introduce a new parameter like ?expand and return meaningful status only when this is used. Using this we won't essentially need to fix integ tests or make change changes in RCA.

We can just update the README for this param usage. @kiranprakash154 @dzane17 Thoughts?

GET localhost:9200/_plugins/_performanceanalyzer/cluster/config
{"batchMetricsRetentionPeriodMinutes": 7, "currentPerformanceAnalyzerClusterState": 19, "shardsPerCollection": 0}

GET localhost:9200/_plugins/_performanceanalyzer/cluster/config?expand
{"currentPerformanceAnalyzerClusterState":{"PerformanceAnalyzerEnabled":true,"RcaEnabled":true,"LoggingEnabled":false,"BatchMetricsEnabled":false,"ThreadContentionMonitoringEnabled":false},"shardsPerCollection":0,"batchMetricsRetentionPeriodMinutes":7}

[dzane17]

As this is a breaking change(as api response is essentially changing), I think we should instead introduce a new parameter like ?expand and return meaningful status only when this is used. Using this we won't essentially need to fix integ tests or make change changes in RCA.

We can just update the README for this param usage. @kiranprakash154 @dzane17 Thoughts?

GET localhost:9200/_plugins/_performanceanalyzer/cluster/config
{"batchMetricsRetentionPeriodMinutes": 7, "currentPerformanceAnalyzerClusterState": 19, "shardsPerCollection": 0}

GET localhost:9200/_plugins/_performanceanalyzer/cluster/config?expand
{"currentPerformanceAnalyzerClusterState":{"PerformanceAnalyzerEnabled":true,"RcaEnabled":true,"LoggingEnabled":false,"BatchMetricsEnabled":false,"ThreadContentionMonitoringEnabled":false},"shardsPerCollection":0,"batchMetricsRetentionPeriodMinutes":7}

I have updated to only return detailed output when ?verbose query param is passed

[sgup432]

Overall LGTM. Just one minor comment.

[sgup432]

Why is this needed?

[dzane17]

It is required by the RestRequest class to mark "verbose" as a consumed parameter. Else it rejects all unknown params.

[sgup432]

This doesn't look right ie to mark it as a param this way. This is a way we fetch "verbose" param rather than mark it, isn't it?
Do you have any other such examples in existing PA codebase?

[dzane17]

See comment on L234 -

performance-analyzer/src/main/java/org/opensearch/performanceanalyzer/http_action/config/PerformanceAnalyzerResourceProvider.java

Lines 230 to 238 in 066cef8

	String redirectEndpoint = request.param("redirectEndpoint");
	String urlScheme = isHttpsEnabled ? "https://" : "http://";
	String redirectBasePath =
	urlScheme + "localhost:" + portNumber + RestConfig.PA_BASE_URI + "/";
	// Need to register all params in OpenSearch request else opensearch throws
	// illegal_argument_exception
	for (String key : request.params().keySet()) {
	request.param(key);
	}

Yes, this method is primarily meant to fetch the value of the verbose param which correspondingly marks it as "consumed." Since we do not care about the actual param value, there is no need to store the output of this call.

[sgup432]

Got it, thanks

[kkhatua]

LGTM. Update the Description to check off the addition of Tests to your feature.

Would also be good to know why the JDK 11 and 17 builds are failing

[opensearch-trigger-bot]

The backport to 1.x failed:

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

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-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-342-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1f35eebbc9b25d46eb2b81463be7e2ba4e7bfc91
# Push it to GitHub
git push --set-upstream origin backport/backport-342-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

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