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.

Sign up for GitHub

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

Cluster config api return verbose PA status #342

Merged
merged 2 commits into from
Jan 10, 2023
Merged

Cluster config api return verbose PA status #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ea865e
Cluster config api return verbose PA status
dzane17 Dec 6, 2022
a3efbd5
Use ?verbose query param
dzane17 Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ GET localhost:9200/_plugins/_performanceanalyzer/config
GET localhost:9200/_plugins/_performanceanalyzer/cluster/config

{"currentPerformanceAnalyzerClusterState":9,"shardsPerCollection":0,"batchMetricsRetentionPeriodMinutes":7}

GET localhost:9200/_plugins/_performanceanalyzer/cluster/config?verbose

{"currentPerformanceAnalyzerClusterState":{"PerformanceAnalyzerEnabled":false,"RcaEnabled":false,"LoggingEnabled":false,"BatchMetricsEnabled":false,"ThreadContentionMonitoringEnabled":false},"shardsPerCollection":0,"batchMetricsRetentionPeriodMinutes":7
```

The default retention period is 7 minutes. However, the cluster owner can adjust this by setting `batch-metrics-retention-period-minutes` in performance-analyzer.properties (note, setting this value will require a restart so that the cluster can read the new value upon startup). The value must be between 1 and 60 minutes (inclusive) — the range is capped like so in order to prevent excessive data retention on the cluster, which would eat up a lot of storage.
Expand Down
1 change: 0 additions & 1 deletion licenses/performanceanalyzer-rca-3.0.0.0-SNAPSHOT.jar.sha1
View file
Open in desktop

This file was deleted.

25 changes: 25 additions & 0 deletions .../performanceanalyzer/config/setting/handler/PerformanceAnalyzerClusterSettingHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
package org.opensearch.performanceanalyzer.config.setting.handler;


import java.util.LinkedHashMap;
import java.util.Map;
import org.opensearch.performanceanalyzer.config.PerformanceAnalyzerController;
import org.opensearch.performanceanalyzer.config.setting.ClusterSettingListener;
import org.opensearch.performanceanalyzer.config.setting.ClusterSettingsManager;
Expand Down Expand Up @@ -34,6 +36,13 @@ public class PerformanceAnalyzerClusterSettingHandler implements ClusterSettingL
.THREAD_CONTENTION_MONITORING_BIT
.ordinal();

static final String PA_ENABLED_KEY = "PerformanceAnalyzerEnabled";
static final String RCA_ENABLED_KEY = "RcaEnabled";
static final String LOGGING_ENABLED_KEY = "LoggingEnabled";
static final String BATCH_METRICS_ENABLED_KEY = "BatchMetricsEnabled";
static final String THREAD_CONTENTION_MONITORING_ENABLED_KEY =
"ThreadContentionMonitoringEnabled";

private final PerformanceAnalyzerController controller;
private final ClusterSettingsManager clusterSettingsManager;

Expand Down Expand Up @@ -136,6 +145,22 @@ public int getCurrentClusterSettingValue() {
return currentClusterSetting;
}

public Map<String, Boolean> getCurrentClusterSettingValueVerbose() {
Map<String, Boolean> statusMap = new LinkedHashMap<String, Boolean>();
statusMap.put(PA_ENABLED_KEY, getPAStateFromSetting(currentClusterSetting.intValue()));
statusMap.put(RCA_ENABLED_KEY, getRcaStateFromSetting(currentClusterSetting.intValue()));
statusMap.put(
LOGGING_ENABLED_KEY, getLoggingStateFromSetting(currentClusterSetting.intValue()));
statusMap.put(
BATCH_METRICS_ENABLED_KEY,
getBatchMetricsStateFromSetting(currentClusterSetting.intValue()));
statusMap.put(
THREAD_CONTENTION_MONITORING_ENABLED_KEY,
getThreadContentionMonitoringStateFromSetting(currentClusterSetting.intValue()));

return statusMap;
}

/**
* Gets the cluster settings from the controller.
*
Expand Down
7 changes: 6 additions & 1 deletion ...search/performanceanalyzer/http_action/config/PerformanceAnalyzerClusterConfigAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ public List<ReplacedRoute> replacedRoutes() {
@Override
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client)
throws IOException {
request.param("verbose");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks

if (request.method() == RestRequest.Method.POST && request.content().length() > 0) {
Map<String, Object> map =
XContentHelper.convertToMap(request.content(), false, XContentType.JSON).v2();
Expand Down Expand Up @@ -200,7 +201,11 @@ protected RestChannelConsumer prepareRequest(final RestRequest request, final No
try {
XContentBuilder builder = channel.newBuilder();
builder.startObject();
builder.field(CURRENT, clusterSettingHandler.getCurrentClusterSettingValue());
builder.field(
CURRENT,
request.paramAsBoolean("verbose", false)
? clusterSettingHandler.getCurrentClusterSettingValueVerbose()
: clusterSettingHandler.getCurrentClusterSettingValue());
builder.field(SHARDS_PER_COLLECTION, nodeStatsSettingHandler.getNodeStatsSetting());
builder.field(
BATCH_METRICS_RETENTION_PERIOD_MINUTES,
Expand Down
41 changes: 41 additions & 0 deletions ...ormanceanalyzer/config/setting/handler/PerformanceAnalyzerClusterSettingHandlerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import static org.mockito.MockitoAnnotations.initMocks;
import static org.powermock.api.mockito.PowerMockito.when;

import java.util.LinkedHashMap;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
Expand All @@ -19,6 +21,16 @@ public class PerformanceAnalyzerClusterSettingHandlerTests {
private static final Boolean DISABLED_STATE = Boolean.FALSE;
private static final Boolean ENABLED_STATE = Boolean.TRUE;

private final Map<String, Boolean> ALL_ENABLED_CLUSTER =
createStatusMap(
ENABLED_STATE, ENABLED_STATE, ENABLED_STATE, ENABLED_STATE, ENABLED_STATE);
private final Map<String, Boolean> ALL_DISABLED_CLUSTER =
createStatusMap(
DISABLED_STATE, DISABLED_STATE, DISABLED_STATE, DISABLED_STATE, DISABLED_STATE);
private final Map<String, Boolean> MIXED_STATUS_CLUSTER =
createStatusMap(
ENABLED_STATE, ENABLED_STATE, DISABLED_STATE, DISABLED_STATE, DISABLED_STATE);

private PerformanceAnalyzerClusterSettingHandler clusterSettingHandler;

@Mock private PerformanceAnalyzerController mockPerformanceAnalyzerController;
Expand All @@ -37,6 +49,8 @@ public void disabledClusterStateTest() {
new PerformanceAnalyzerClusterSettingHandler(
mockPerformanceAnalyzerController, mockClusterSettingsManager);
assertEquals(0, clusterSettingHandler.getCurrentClusterSettingValue());
assertEquals(
ALL_DISABLED_CLUSTER, clusterSettingHandler.getCurrentClusterSettingValueVerbose());
}

@Test
Expand All @@ -47,6 +61,8 @@ public void enabledClusterStateTest() {
new PerformanceAnalyzerClusterSettingHandler(
mockPerformanceAnalyzerController, mockClusterSettingsManager);
assertEquals(31, clusterSettingHandler.getCurrentClusterSettingValue());
assertEquals(
ALL_ENABLED_CLUSTER, clusterSettingHandler.getCurrentClusterSettingValueVerbose());
}

@Test
Expand All @@ -57,6 +73,8 @@ public void paDisabledClusterStateTest() {
new PerformanceAnalyzerClusterSettingHandler(
mockPerformanceAnalyzerController, mockClusterSettingsManager);
assertEquals(0, clusterSettingHandler.getCurrentClusterSettingValue());
assertEquals(
ALL_DISABLED_CLUSTER, clusterSettingHandler.getCurrentClusterSettingValueVerbose());
}

@Test
Expand All @@ -67,8 +85,12 @@ public void updateClusterStateTest() {
new PerformanceAnalyzerClusterSettingHandler(
mockPerformanceAnalyzerController, mockClusterSettingsManager);
assertEquals(3, clusterSettingHandler.getCurrentClusterSettingValue());
assertEquals(
MIXED_STATUS_CLUSTER, clusterSettingHandler.getCurrentClusterSettingValueVerbose());
clusterSettingHandler.onSettingUpdate(0);
assertEquals(0, clusterSettingHandler.getCurrentClusterSettingValue());
assertEquals(
ALL_DISABLED_CLUSTER, clusterSettingHandler.getCurrentClusterSettingValueVerbose());
}

private void setControllerValues(
Expand All @@ -86,4 +108,23 @@ private void setControllerValues(
when(mockPerformanceAnalyzerController.isThreadContentionMonitoringEnabled())
.thenReturn(threadContentionMonitoringEnabled);
}

private Map<String, Boolean> createStatusMap(
final Boolean paEnabled,
final Boolean rcaEnabled,
final Boolean loggingEnabled,
final Boolean batchMetricsEnabled,
final Boolean threadContentionMonitoringEnabled) {
Map<String, Boolean> statusMap = new LinkedHashMap<String, Boolean>();
statusMap.put(PerformanceAnalyzerClusterSettingHandler.PA_ENABLED_KEY, paEnabled);
statusMap.put(PerformanceAnalyzerClusterSettingHandler.RCA_ENABLED_KEY, rcaEnabled);
statusMap.put(PerformanceAnalyzerClusterSettingHandler.LOGGING_ENABLED_KEY, loggingEnabled);
statusMap.put(
PerformanceAnalyzerClusterSettingHandler.BATCH_METRICS_ENABLED_KEY,
batchMetricsEnabled);
statusMap.put(
PerformanceAnalyzerClusterSettingHandler.THREAD_CONTENTION_MONITORING_ENABLED_KEY,
threadContentionMonitoringEnabled);
return statusMap;
}
}