78 collect stats for usage and health #79
Conversation
…ts and cache. Signed-off-by: Johannes Peter <[email protected]>
JohannesDaniel
requested review from
gsingers,
macohen and
sstults
as code owners
…s and caches into query implementations Signed-off-by: Johannes Peter <[email protected]>
JohannesDaniel
force-pushed
the
78-collect-stats-for-usage-and-health
branch
from
8a1ea9a to
7b5ac91
Compare
Signed-off-by: Johannes Peter <[email protected]>
| @Override | ||
| public Map<String, Map<String, Object>> get() { | ||
| Map<String, Map<String, Object>> values = new HashMap<>(); | ||
| values.put("feature", getCacheStats(caches.featureCache())); |
There was a problem hiding this comment.
Consider using an enum for cache types instead of hardcoded strings?
Then we can refactor this method as
@Override
public Map<String, Map<String, Object>> get() {
Map<String, Map<String, Object>> values = new HashMap<>();
for (CacheType type : CacheType.values()) {
values.put(type.getValue(), getCacheStats(getCacheForType(type)));
}
return Collections.unmodifiableMap(values);
}
There was a problem hiding this comment.
Due to the way the caching is implemented as of now, I do not see the benefit of having an enum here, as this will still require a direct mapping between the enum and the method called to get the corresponding metrics, so the code is not going to be simplified/generified:
caches.featureCache()
caches.featureSetCache()
caches.modelCache()However, I will pull the hard-coded strings out of the method and refactor them to static class variables.
| return _doToQuery(context); | ||
| } catch (Exception e) { | ||
| ltrStats.getStat(StatName.LTR_REQUEST_ERROR_COUNT.getName()).increment(); | ||
| throw e; |
There was a problem hiding this comment.
This is just a wrapper to collect stats about exceptions but does not initiate or handle the exceptions. I would expect them to be logged where they are initiated or handled, otherwise this might lead to redundant logging.
There was a problem hiding this comment.
Got it, sounds good if upper layer already have logging for such case
| return _createWeight(searcher, scoreMode, boost); | ||
| } catch (Exception e) { | ||
| ltrStats.getStat(StatName.LTR_REQUEST_ERROR_COUNT.getName()).increment(); | ||
| throw e; |
Signed-off-by: Johannes Peter <[email protected]>
Signed-off-by: Johannes Peter <[email protected]>
…bles Signed-off-by: Johannes Peter <[email protected]>
Signed-off-by: Johannes Peter <[email protected]>
… class variables for class CacheStatsOnNodeSupplier Signed-off-by: Johannes Peter <[email protected]>
Signed-off-by: Johannes Peter <[email protected]> (cherry picked from commit c19e3a3)
Description
Collecting stats about requests in total, requests with errors and cache usage.
Issues Resolved
#78
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.