-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Add metric for total downsampling latency #106747
Conversation
@elasticsearchmachine run elasticsearch-ci/part-1 |
@elasticsearchmachine test this |
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
@@ -173,6 +177,12 @@ public TransportDownsampleAction( | |||
this.threadContext = threadPool.getThreadContext(); | |||
this.taskQueue = clusterService.createTaskQueue("downsample", Priority.URGENT, STATE_UPDATE_TASK_EXECUTOR); | |||
this.persistentTasksService = persistentTasksService; | |||
this.downsampleMetrics = downsampleMetrics; | |||
this.startTime = client.threadPool().relativeTimeInMillis(); |
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.
Just for my education - we actually create new instance of action for every invocation?
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.
Good question. My understanding is that actions are initialized once in the plugin, then their methods (masterOperation
) are called per incoming request.
...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java
Outdated
Show resolved
Hide resolved
...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java
Outdated
Show resolved
Hide resolved
@@ -30,6 +30,7 @@ | |||
public class DownsampleMetrics extends AbstractLifecycleComponent { | |||
|
|||
public static final String LATENCY_SHARD = "es.tsdb.downsample.latency.shard.histogram"; | |||
public static final String LATENCY_MASTER = "es.tsdb.downsample.latency.master.histogram"; |
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.
The master
part is leaking implementation details that may change in the future. I think that maybe api latency or operation latency is a better name here. Wdyt?
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.
Switched to total
.
...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java
Outdated
Show resolved
Hide resolved
@@ -459,12 +470,16 @@ public void onResponse(PersistentTasksCustomMetadata.PersistentTask<PersistentTa | |||
if (countDown.decrementAndGet() == 0) { | |||
logger.info("All downsampling tasks completed [" + numberOfShards + "]"); | |||
updateTargetIndexSettingStep(request, listener, sourceIndexMetadata, downsampleIndexName, parentTask); | |||
downsampleMetrics.recordLatencyMaster(getDurationInMillis(), DownsampleMetrics.ActionStatus.SUCCESS); |
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.
Do we want to take into account the time it takes to perform the refresh and force merge? If so then we should move this to ForceMergeActionListener
.
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.
Moved, and added also in the failure handlers, ptal.
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.
👍
This metric tracks latency on master side, in addition to per-shard metrics that were added in #106632