Implementation_of_Error_Metric_for_Rest_Actions #4567
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Author Name <vjg@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
| @@ -79,6 +81,7 @@ public NodesStatsRequest(StreamInput in) throws IOException { | |||
| optionallyAddMetric(in.readBoolean(), Metric.ADAPTIVE_SELECTION.metricName()); | |||
| } else { | |||
| requestedMetrics.addAll(in.readStringList()); | |||
| restActionsFilters.addAll(in.readStringList()); | |||
There was a problem hiding this comment.
What happens if no filters are provided?
There was a problem hiding this comment.
if filters not provided, we'll get status count for all rest rest actions having count > 0
| * @opensearch.internal | ||
| */ | ||
| public class RestActionsStats implements Writeable, ToXContentFragment { | ||
| private Map<String, Map<Integer, Integer>> restActionsStatusCount; |
There was a problem hiding this comment.
Can we use Long here for the count?
There was a problem hiding this comment.
I think status count won't go beyond INT range, so making it LONG, will cause memory wastage.
| */ | ||
| public class RestActionsStats implements Writeable, ToXContentFragment { | ||
| private Map<String, Map<Integer, Integer>> restActionsStatusCount; | ||
| private Map<Integer, Integer> restActionsStatusTotalCount; |
There was a problem hiding this comment.
- Can you add documentation for the fields here?
- Is the key supposed to denote the status code?
|
Could you help with the sample request and response structure for this? |
| * | ||
| * @opensearch.internal | ||
| */ | ||
| public class RestActionsService { |
There was a problem hiding this comment.
Can we see if a name like RestActionsStatsService is better suited for this, since this only deals with stats for the rest actions and no other operations?
There was a problem hiding this comment.
Indeed, "RestActionsStatsService" better suits here and gives a clear info about class as well.
I came up with another one also -"RestActionsStatsCountService" , will it be ok?
| private Map<Integer, Integer> getRestActionsStatusTotalCount(Map<String, Map<Integer, Integer>> mapHandlerStatusCount) { | ||
| final Map<Integer, Integer> totalStatusCount = new TreeMap<>(); | ||
| mapHandlerStatusCount.entrySet().stream().flatMap(entry -> entry.getValue().entrySet().stream()).forEach(entry -> { | ||
| Integer count = totalStatusCount.get(entry.getKey()); | ||
| if (null == count) count = 0; | ||
| count += entry.getValue(); | ||
| totalStatusCount.put(entry.getKey(), count); | ||
| }); | ||
| return totalStatusCount; | ||
| } |
There was a problem hiding this comment.
Please ensure to have unit tests tests for null values
There was a problem hiding this comment.
I'll cross check, if not added, I'll add them.
| private void increment(Map<Integer, Integer> statusCount, int status) { | ||
| Integer count = statusCount.get(status); | ||
| if (null == count) count = 0; | ||
| statusCount.put(status, ++count); | ||
| } |
There was a problem hiding this comment.
We need to make it thread safe. Can we use AtomicLong here to ensure we don't skip any count?
There was a problem hiding this comment.
Also, please see if we can tests to verify thread safety across operations
There was a problem hiding this comment.
Rather using AtomicLong , we can make the Map thread-safe?
| if (handler.getName() == null) { | ||
| throw new IllegalArgumentException("handler of type [" + handler.getClass().getName() + "] does not have a name"); | ||
| } | ||
| handlers.putIfAbsent(handler.getName(), new TreeMap<>()); |
There was a problem hiding this comment.
Shouldn't the inner map be thread safe as well?
| if (handler.getName() == null) { | ||
| throw new IllegalArgumentException("handler of type [" + handler.getClass().getName() + "] does not have a name"); | ||
| } | ||
| handlers.putIfAbsent(handler.getName(), new TreeMap<>()); |
There was a problem hiding this comment.
Fail if already present as well?
There was a problem hiding this comment.
No, putIfAbsent only puts value if no entry exists for the key and returns null, if key is already there it doesn't replace exiting value but returns existing value.
ex:
map.putIfAbsent("exist", "no"); --- returns null
map.putIfAbsent("exist", "yes"); --- returns "no"
there is no error or exception
| if (restHandler instanceof BaseRestHandler) restActionsService.putStatus( | ||
| ((BaseRestHandler) restHandler).getName(), | ||
| response.status() | ||
| ); |
There was a problem hiding this comment.
[Nit] Can we re-format this snippet to be aligned with the convention followed in the repository?
There was a problem hiding this comment.
I already formatted code using ./gradlew spotlessApply.
I guess had the code not formatted using above command, it couldn't be pushed.
| if (metrics.contains("rest_actions") && request.hasParam("rest_actions_filters")) { | ||
| nodesStatsRequest.addRestActionsFilters(request.paramAsStringArray("rest_actions_filters", null)); | ||
| } else if (request.hasParam("rest_actions_filters")) { |
There was a problem hiding this comment.
Do we need validations on the provided values for rest_actions_filters as well?
There was a problem hiding this comment.
I did have thoughts about it while coding but the thing is we need to maintain the list of all rest actions and whenever a new rest action would be introduced, it must be added to that list. So I think it's un-necessary. As per current implementation if filter contain invalid action name it won't be returned as response.
Or we have to use RestActionsService (class in which all handlers are being registered) ref here, again it creates un-necessary dependency which I think is not a good design.
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Author Name <vjg@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
|
Closing this PR , new PR raised by Prabhat #5334 |
Description
Complete implementation of issue: #4401
Issues Resolved
#4401
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.