-
Notifications
You must be signed in to change notification settings - Fork 69
Add Indexing Latency, Indexing and Search Rate metric per shard #797
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.
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Arpit Patawat <[email protected]>
initializeMetricsIfNeeded(); | ||
LOG.debug("Executing collect metrics for RTFShardOperationRateCollector"); | ||
|
||
long currentTimeInMillis = System.currentTimeMillis(); |
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.
What will be value in this scenario - 100 request came on a shard and 10:01:00 and another 100 (so in total 200) at 10:05:00. Last collection happened at 10:04:55 and now its happening at 10:05:00. So minutesSinceLastCollection would be (5000/60000 = .083) and the rate would be (200 - 100)/0.083 = 1204.81. Is this the right calculation or am missing something?
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.
Will change the rate to 5 second and remove the difference between the last collection time to make sure it will only publish the current diff, despite the time difference with last collection.
"Indexing operations per minute per shard", | ||
MetricUnits.RATE.toString()); | ||
|
||
searchRateHistogram = |
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.
Rename as this is not histo
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.
ok
.../org/opensearch/performanceanalyzer/collectors/telemetry/RTFShardOperationRateCollector.java
Show resolved
Hide resolved
indexingRateHistogram.add(indexingRatePerMinute, tags); | ||
} | ||
|
||
private void processSearchOperations( |
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.
Can you add a sample metrics for this as to how the dimensions are looking like
Signed-off-by: Arpit Patawat <[email protected]>
Signed-off-by: Arpit Patawat <[email protected]>
|
||
fetchPhaseHistogram.record( | ||
fetchTimeInMills, createTags(searchContext, SHARD_FETCH_PHASE, false)); | ||
queryPlusFetchPhaseHistogram.record( |
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.
Should we add the phase here? It defeats the purpose of this metric.
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.
Updated the metric tags to have same value for phase, so that both metrics are the same.
Signed-off-by: Arpit Patawat <[email protected]>
@@ -38,6 +39,7 @@ public class RTFPerformanceAnalyzerSearchListener | |||
LogManager.getLogger(RTFPerformanceAnalyzerSearchListener.class); | |||
private static final String SHARD_FETCH_PHASE = "shard_fetch"; | |||
private static final String SHARD_QUERY_PHASE = "shard_query"; | |||
private static final String SHARD_QUERY_PLUS_FETCH_PHASE = "shard_query_plus_fetch"; |
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.
Should we just call search.latency ?
@@ -38,6 +39,7 @@ public class RTFPerformanceAnalyzerSearchListener | |||
LogManager.getLogger(RTFPerformanceAnalyzerSearchListener.class); | |||
private static final String SHARD_FETCH_PHASE = "shard_fetch"; |
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.
Please change to Otel standards shard.fetch this is same for all dimensions added
Description
This commit will add 3 metrics per shard which are -
These will be helpful to get detailed insights at shard level.
Related Issues
None
Check List
--signoff
.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.