Add slicing mode and fix operation-level reindex metrics by samxbr · Pull Request #143415 · elastic/elasticsearch

samxbr · 2026-03-02T19:12:20Z

Adds a es_reindex_slicing_mode dimension to existing reindex metrics to capture what type of slicing is being used. Also fixed the reindex metrics to be operation-level, meaning each API call to reindex will count as a single reindex metric. Reindex with multiple slices will only emit a single metric instead of one per slice.

Closes #138258
Closes https://github.com/elastic/elasticsearch-team/issues/2375

elasticsearchmachine · 2026-03-03T00:24:47Z

Pinging @elastic/es-distributed (Team:Distributed)

PeteGillinElastic

Thanks Sam. This LGTM as it stands. But you have a merge conflict, and if that's with Szymon's change to implement relocation with slicing then I suspect that there might be some non-trivial work to be done in Reindexer to resolve it. (You already have a bunch of — unavoidably — fiddly stuff with multiply nested callbacks, and I have a nasty feeling that stuff has changed under you.) I think it's better if we hold off on approval until that's done.

Once you've dealt with the conflict, it might also be a good idea to ping Szymon and get him to look specifically at the changes to the callbacks in Reindexer as he probably knows that better than me.

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java

samxbr · 2026-03-04T23:20:14Z

Thanks Sam. This LGTM as it stands. But you have a merge conflict, and if that's with Szymon's change to implement relocation with slicing then I suspect that there might be some non-trivial work to be done in Reindexer to resolve it. (You already have a bunch of — unavoidably — fiddly stuff with multiply nested callbacks, and I have a nasty feeling that stuff has changed under you.) I think it's better if we hold off on approval until that's done.

Once you've dealt with the conflict, it might also be a good idea to ping Szymon and get him to look specifically at the changes to the callbacks in Reindexer as he probably knows that better than me.

Yeah I did expect there will be some conflict with Szymon's PR. @szybia to have a look as well

PeteGillinElastic

This LGTM, but I'd also like @szybia to weigh in.

...dex/src/internalClusterTest/java/org/elasticsearch/index/reindex/ReindexPluginMetricsIT.java

szybia

initial review, haven't scanned tests yet 🚀

...t/src/internalClusterTest/java/org/elasticsearch/reindex/management/ReindexRelocationIT.java

modules/reindex/src/main/java/org/elasticsearch/reindex/Reindexer.java

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java

modules/reindex/src/test/java/org/elasticsearch/reindex/ReindexerTests.java

modules/reindex/src/test/java/org/elasticsearch/reindex/ReindexMetricsTests.java

szybia

looks great! thinking last batch of comments

modules/reindex/src/main/java/org/elasticsearch/reindex/Reindexer.java

...t/src/internalClusterTest/java/org/elasticsearch/reindex/management/ReindexRelocationIT.java

szybia · 2026-03-05T18:06:00Z

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java

    public static final String ATTRIBUTE_VALUE_SOURCE_LOCAL = "local";
    public static final String ATTRIBUTE_VALUE_SOURCE_REMOTE = "remote";

+    public static final String ATTRIBUTE_NAME_SLICING_MODE = "es_reindex_slicing_mode";


strikes me as a non-consistent name for an attribute, given the other names, look more like a metric name, thoughts?

Suggested change

public static final String ATTRIBUTE_NAME_SLICING_MODE = "es_reindex_slicing_mode";

public static final String ATTRIBUTE_NAME_SLICING_MODE = "slicing_mode";

Metric attribute names are validated in MetricValidator, es(_<segment>)+ is the recommended naming convention according to NAMING.md

The validation and convention are introduced after the earlier reindex metrics, ideally we should migrate other metrics to the new convention too later

Yeah, the es_ prefix is required now, as I discovered the hard way :-). The previous ones have been grandfathered in as exceptions in the validator, but we shouldn't add more.

We could probably drop the reindex though, as this thing is only used as an attributed to metrics with reindex in their names, i.e. es_slicing_mode?

Always use es_ as the root segment to easily discover ES attributes. Follow with a module name, team or area of code, e.g. snapshot, repositories, indices, threadpool using existing terminology (whether singular and plural)

So according to their convention, es_reindex_slicing_mode feels a little bit better with a module/area of code prefix

Always use es_ as the root

where is this written? (curious)

Thanks for that quote Sam, I agree that including reindex makes sense. I wasn't going off any documentation, I was going off the exception that got thrown when I didn't have the es_ prefix and looking at the regex that was being enforced by that validation code.

Always use es_ as the root

where is this written? (curious)

https://github.com/elastic/elasticsearch/blob/main/modules/apm/NAMING.md

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java

modules/reindex/src/main/java/org/elasticsearch/reindex/Reindexer.java

modules/reindex/src/test/java/org/elasticsearch/reindex/ReindexerTests.java

szybia

looks great! ty

szybia · 2026-03-06T11:26:46Z

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java

    public static final String ATTRIBUTE_VALUE_SOURCE_LOCAL = "local";
    public static final String ATTRIBUTE_VALUE_SOURCE_REMOTE = "remote";

+    public static final String ATTRIBUTE_NAME_SLICING_MODE = "es_reindex_slicing_mode";


Always use es_ as the root

where is this written? (curious)

)

samxbr added 5 commits March 2, 2026 13:27

WIP

f4b655a

unit tests

290a8cf

add IT

b224f6a

minor

d82ebec

Merge branch 'main' into reindex/metrics

e3188bd

elasticsearchmachine added the v9.4.0 label Mar 2, 2026

minor test

180b17d

samxbr added :Distributed/Reindex Issues relating to reindex that are not caused by issues further down >non-issue >bug and removed >non-issue labels Mar 2, 2026

fix metric name

8e08af8

samxbr changed the title ~~Add slicing_mode and fix operation-level reindex metrics~~ Add slicing mode and fix operation-level reindex metrics Mar 2, 2026

samxbr marked this pull request as ready for review March 3, 2026 00:24

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Mar 3, 2026

samxbr requested a review from PeteGillinElastic March 3, 2026 18:16

PeteGillinElastic reviewed Mar 4, 2026

View reviewed changes

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java Outdated Show resolved Hide resolved

modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java Outdated Show resolved Hide resolved

samxbr added 2 commits March 4, 2026 13:46

Merge branch 'main' into reindex/metrics

0af10ad

Fix relocated sliced metrics

1968c60

samxbr requested a review from szybia March 4, 2026 23:20

PeteGillinElastic reviewed Mar 5, 2026

View reviewed changes

...dex/src/internalClusterTest/java/org/elasticsearch/index/reindex/ReindexPluginMetricsIT.java Show resolved Hide resolved

szybia reviewed Mar 5, 2026

View reviewed changes

modules/reindex/src/test/java/org/elasticsearch/reindex/ReindexerTests.java Outdated Show resolved Hide resolved

modules/reindex/src/test/java/org/elasticsearch/reindex/ReindexMetricsTests.java Outdated Show resolved Hide resolved

samxbr added 2 commits March 5, 2026 11:01

adress comments

aea2eff

Merge branch 'main' into reindex/metrics

f65b692

szybia reviewed Mar 5, 2026

View reviewed changes

samxbr added 2 commits March 5, 2026 13:23

add tests

0ffebf5

fix

dadd1fb

samxbr added 3 commits March 5, 2026 14:38

minor

90ae2bb

fix listener

8bcee2c

Merge branch 'main' into reindex/metrics

9bf4d1e

szybia approved these changes Mar 6, 2026

View reviewed changes

samxbr merged commit 1a56af0 into elastic:main Mar 6, 2026
35 checks passed

prwhelan mentioned this pull request Mar 6, 2026

[ML] Wait for cluster state in test #143767

Merged

sidosera pushed a commit to sidosera/elasticsearch that referenced this pull request Mar 6, 2026

Add slicing mode and fix operation-level reindex metrics (elastic#143415

824baa0

)

prwhelan mentioned this pull request Mar 9, 2026

[Transform] Disable PIT for CPS #143876

Closed

	public static final String ATTRIBUTE_NAME_SLICING_MODE = "es_reindex_slicing_mode";
	public static final String ATTRIBUTE_NAME_SLICING_MODE = "slicing_mode";

Conversation

samxbr commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 3, 2026

Uh oh!

PeteGillinElastic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samxbr commented Mar 4, 2026

Uh oh!

PeteGillinElastic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szybia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szybia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szybia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samxbr commented Mar 2, 2026 •

edited

Loading