-
Notifications
You must be signed in to change notification settings - Fork 56
Add stats to track knn request counts #89
Changes from all commits
399ee4c
1a2ccff
308d051
824a8b3
beda396
06cd43e
45cd741
69a9c89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
|
||
import com.amazon.opendistroforelasticsearch.knn.index.KNNQueryResult; | ||
import com.amazon.opendistroforelasticsearch.knn.index.util.NmsLibVersion; | ||
import com.amazon.opendistroforelasticsearch.knn.plugin.stats.KNNCounter; | ||
|
||
import java.io.File; | ||
import java.io.IOException; | ||
|
@@ -70,7 +71,7 @@ public long getIndexSize() { | |
public KNNQueryResult[] queryIndex(final float[] query, final int k) throws IOException { | ||
Lock readLock = readWriteLock.readLock(); | ||
readLock.lock(); | ||
|
||
KNNCounter.GRAPH_QUERY_REQUESTS.increment(); | ||
try { | ||
if (this.isClosed) { | ||
throw new IOException("Index is already closed"); | ||
|
@@ -84,6 +85,9 @@ public KNNQueryResult[] run() { | |
} | ||
); | ||
|
||
} catch (Exception ex) { | ||
KNNCounter.GRAPH_QUERY_ERRORS.increment(); | ||
throw new RuntimeException("Unable to query the index: " + ex); | ||
Comment on lines
+88
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a thought, why not we just rely on load_exception_count metric from cache stats. This seem to track number of exceptions while loading graph which will be invoked during queries? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using load_exception_count would only count exceptions for loading the graph into memory, not the actual query of the graph. Adding the metric here allows us to check if the library query of the graph fails. In your opinion, should this metric track the number of query errors where a query is a call to the ES search API for knn, or for a query where a query is a call to the library function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense. It should track the number of query errors. |
||
} finally { | ||
readLock.unlock(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package com.amazon.opendistroforelasticsearch.knn.plugin.stats; | ||
|
||
import java.util.concurrent.atomic.AtomicLong; | ||
|
||
/** | ||
* Contains a map of counters to keep track of different values | ||
*/ | ||
public enum KNNCounter { | ||
GRAPH_QUERY_ERRORS("graph_query_errors"), | ||
GRAPH_QUERY_REQUESTS("graph_query_requests"), | ||
GRAPH_INDEX_ERRORS("graph_index_errors"), | ||
GRAPH_INDEX_REQUESTS("graph_index_requests"), | ||
KNN_QUERY_REQUESTS("knn_query_requests"); | ||
|
||
private String name; | ||
private AtomicLong count; | ||
|
||
/** | ||
* Constructor | ||
* | ||
* @param name name of the counter | ||
*/ | ||
KNNCounter(String name) { | ||
this.name = name; | ||
this.count = new AtomicLong(0); | ||
} | ||
|
||
/** | ||
* Get name of counter | ||
* | ||
* @return name | ||
*/ | ||
public String getName() { | ||
return name; | ||
} | ||
|
||
/** | ||
* Get the value of count | ||
* | ||
* @return count | ||
*/ | ||
public Long getCount() { | ||
return count.get(); | ||
} | ||
|
||
/** | ||
* Increment the value of a counter | ||
*/ | ||
public void increment() { | ||
count.getAndIncrement(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/* | ||
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package com.amazon.opendistroforelasticsearch.knn.plugin.stats.suppliers; | ||
|
||
import com.amazon.opendistroforelasticsearch.knn.plugin.stats.KNNCounter; | ||
|
||
import java.util.function.Supplier; | ||
|
||
/** | ||
* Supplier for stats that need to keep count | ||
*/ | ||
public class KNNCounterSupplier implements Supplier<Long> { | ||
private KNNCounter knnCounter; | ||
|
||
/** | ||
* Constructor | ||
* | ||
* @param knnCounter KNN Plugin Counter | ||
*/ | ||
public KNNCounterSupplier(KNNCounter knnCounter) { | ||
this.knnCounter = knnCounter; | ||
} | ||
|
||
@Override | ||
public Long get() { | ||
return knnCounter.getCount(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package com.amazon.opendistroforelasticsearch.knn.plugin.stats; | ||
|
||
import org.elasticsearch.test.ESTestCase; | ||
|
||
public class KNNCounterTests extends ESTestCase { | ||
public void testGetName() { | ||
assertEquals(StatNames.GRAPH_QUERY_ERRORS.getName(), KNNCounter.GRAPH_QUERY_ERRORS.getName()); | ||
} | ||
|
||
public void testCount() { | ||
assertEquals((Long) 0L, KNNCounter.GRAPH_QUERY_ERRORS.getCount()); | ||
|
||
for (long i = 0; i < 100; i++) { | ||
KNNCounter.GRAPH_QUERY_ERRORS.increment(); | ||
assertEquals((Long) (i+1), KNNCounter.GRAPH_QUERY_ERRORS.getCount()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package com.amazon.opendistroforelasticsearch.knn.plugin.stats.suppliers; | ||
|
||
import com.amazon.opendistroforelasticsearch.knn.plugin.stats.KNNCounter; | ||
import org.elasticsearch.test.ESTestCase; | ||
|
||
public class KNNCounterSupplierTests extends ESTestCase { | ||
public void testNormal() { | ||
KNNCounterSupplier knnCounterSupplier = new KNNCounterSupplier(KNNCounter.GRAPH_QUERY_REQUESTS); | ||
assertEquals((Long) 0L, knnCounterSupplier.get()); | ||
KNNCounter.GRAPH_QUERY_REQUESTS.increment(); | ||
assertEquals((Long) 1L, knnCounterSupplier.get()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we add metrics for counting KNNQueries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Will add