This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
Batch Metrics API #159
Merged
Batch Metrics API #159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ricardolstephen
force-pushed
the
batch-metrics-api-v3
branch
from
9686ab8
to
0f90bf4
Compare
ricardolstephen
requested review from
ktkrg,
adityaj1107,
vigyasharma,
yojs and
sendkb
yojs
suggested changes
Aug 14, 2020
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Show resolved
Hide resolved
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Outdated
Show resolved
Hide resolved
ricardolstephen
force-pushed
the
batch-metrics-api-v3
branch
from
3f927dd
to
ef67704
Compare
sidheart
previously approved these changes
Aug 27, 2020
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Show resolved
Hide resolved
| private static final String ENABLED = "enabled"; | ||
| private static final String SHARDS_PER_COLLECTION = "shardsPerCollection"; | ||
| private static final String CURRENT = "currentPerformanceAnalyzerClusterState"; | ||
| private static final String NAME = "PerformanceAnalyzerClusterConfigAction"; | ||
| private static final String BATCH_METRICS_RETENTION_PERIOD_MINUTES = "batchMetricsRetentionPeriodMinutes"; |
There was a problem hiding this comment.
We aren't adding logic to update this via the API then?
There was a problem hiding this comment.
No, this will be a read-only parameter that the cluster owner can modify.
vigyasharma
previously approved these changes
Aug 31, 2020
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Outdated
Show resolved
Hide resolved
| @@ -95,13 +107,15 @@ public int getCurrentClusterSettingValue() { | |||
| * @return the cluster setting value | |||
| */ | |||
| private Integer initializeClusterSettingValue( | |||
There was a problem hiding this comment.
Have we tested this across 1/ multi-node scenarios, 2/ clusters with nodes on different versions of this code (one node with this change and another on a version before this change) ?
There was a problem hiding this comment.
Ran these tests manually. See the test list in opendistro-for-elasticsearch/performance-analyzer-rca#315
yojs
reviewed
Sep 2, 2020
README.md
Outdated
|
|
||
| Note, the maximum number of datapoints that a single query can request for via API is capped at 100,800 datapoints. If a query exceeds this limit, an error is returned. The query parameters can be adjusted on such queries to request for fewer datapoints at a time. | ||
|
|
||
| The retention period for batch metrics can be adjusted by setting batch-metrics-retention-period-minutes in performance-analyzer.properties. The default value is 7, and values can range from 1 to 60 (inclusive). |
There was a problem hiding this comment.
we should be explicit that a really high value of the batch-metrics-retention-period-minutes will lead to heavy disk consumption. We should also have a max this can be set to, to avoid mistakes of setting it too high and then a read through the rest API crippling the cluster.
There was a problem hiding this comment.
We already have a bounding range (1-60 inclusive). Will add the note about the disk consumption.
ricardolstephen
dismissed stale reviews from vigyasharma and sidheart
via
907bbc7
sidheart
previously approved these changes
Sep 8, 2020
vigyasharma
previously approved these changes
Sep 8, 2020
yojs
reviewed
Sep 8, 2020
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Outdated
Show resolved
Hide resolved
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Show resolved
Hide resolved
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Show resolved
Hide resolved
...ro/elasticsearch/performanceanalyzer/http_action/config/PerformanceAnalyzerConfigAction.java
Show resolved
Hide resolved
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Outdated
Show resolved
Hide resolved
yojs
suggested changes
Sep 9, 2020
vigyasharma
previously approved these changes
Sep 10, 2020
yojs
suggested changes
Sep 10, 2020
...mazon/opendistro/elasticsearch/performanceanalyzer/config/PerformanceAnalyzerController.java
Outdated
Show resolved
Hide resolved
yojs
approved these changes
Sep 10, 2020
vigyasharma
approved these changes
Sep 10, 2020
This was referenced Sep 10, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
opendistro-for-elasticsearch/performance-analyzer-rca#335
Description of changes:
Batch metrics API
Associated RCA change: opendistro-for-elasticsearch/performance-analyzer-rca#315
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.