Change the approach to find the APM Agent property file in test by mamazzol · Pull Request #144928 · elastic/elasticsearch

mamazzol · 2026-03-25T13:25:35Z

Change the approach to find the APM Agent property file in tests to replicate what we do when deleting the file at startup.

Closes: #144357
Closes: #144358

elasticsearchmachine · 2026-03-25T13:26:01Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

coderabbitai · 2026-03-25T13:31:52Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Tests were refactored so the APM properties extractor accepts a JVM options list, parses the -javaagent: argument to derive the =c= config path via a new findApmConfigPath helper, verifies the target is a regular file whose filename matches Node.APM_PROPFILE_REGEX, and loads it; missing matches now throw an AssertionError that includes the full options list. Node gained the public constant APM_PROPFILE_REGEX. muted-tests.yml adds a mute for org.elasticsearch.reindex.management.ReindexGetRelocationIT#testGetReindexFollowsRelocation; existing APMJvmOptionsTests mutes remain.

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	❓ Inconclusive	The muted-tests.yml entry for ReindexGetRelocationIT appears unrelated to APM test stabilization; other changes align with fixing the intermittent APMJvmOptionsTests failure.	Clarify why ReindexGetRelocationIT (issue `#144364`) was muted in this PR; verify it is not an accidental inclusion unrelated to issue `#144357`.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR directly addresses issue `#144357` by refactoring test logic to locate APM config files using the same approach as production code, mitigating intermittent directory-vs-file IO errors.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

🛠️ Update Documentation: Commit on current branch
🛠️ Update Documentation: Create PR

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

prdoyle · 2026-03-25T14:20:30Z

I think I need more context on this one. Why was the old logic wrong?

mamazzol · 2026-03-25T14:57:51Z

I think I need more context on this one. Why was the old logic wrong?

Based on the test failure, it still seems to pick up a directory every once in a while (which is surprising, but that's what the message says). So I changed the logic to replicate what we do in Production, so that it's tried and tested

mark-vieira

Is this a side effect of the CLI launcher changes? If so, what exactly is different here?

mamazzol · 2026-03-25T15:49:13Z

Is this a side effect of the CLI launcher changes? If so, what exactly is different here?

I don't think so. This current logic was brought in to fix a similar failure, see #144350

I reproduced that locally by running the test enough times, and it seems to have fixed it, but apparently it's still happening. So I am trying a different approach.

prdoyle

Sorry, I'm still not quite following. Could you link the that "what we do when deleting the file at startup" so I can follow the reasoning?

prdoyle · 2026-03-25T16:49:58Z

...ribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java

+                int configIndex = inputArgument.lastIndexOf("=c=");
+                if (configIndex > 0) {
+                    final Path apmConfig = PathUtils.get(inputArgument.substring(configIndex + 3));
+                    if (Files.isRegularFile(apmConfig) && apmConfig.getFileName().toString().matches("^\\.elstcapm\\..*\\.tmp$")) {


Why did this go back to being a regex?

Replying to both at the same time, here is the logic I am referring to

elasticsearch/server/src/main/java/org/elasticsearch/node/Node.java

Line 215 in 85fce73

public static void deleteTemporaryApmConfig(JvmInfo jvmInfo, BiConsumer<Exception, Path> errorHandler) {

This uses the regex so I put it back this way.

Ah I see! Maybe let's make that a public constant that can be shared in the code rather than duplicating it? That would make the code itself answer both questions!.

…tic#144928) # Conflicts: # muted-tests.yml

mamazzol added 2 commits March 25, 2026 13:22

Change approach to find APM property file in test

7695b96

Unmute test

413d8ab

mamazzol requested a review from a team as a code owner March 25, 2026 13:25

mamazzol added >test Issues or PRs that are addressing/adding tests Team:Core/Infra Meta label for core/infra team :Core/Infra/Metrics Metrics and metering infrastructure labels Mar 25, 2026

elasticsearchmachine added the v9.4.0 label Mar 25, 2026

mamazzol changed the title ~~Ci 144357~~ Change the approach to find the APM Agent property file in test Mar 25, 2026

Merge branch 'main' into ci-144357

84fdaf6

Merge branch 'main' into ci-144357

e836f2c

mark-vieira reviewed Mar 25, 2026

View reviewed changes

prdoyle reviewed Mar 25, 2026

View reviewed changes

mamazzol added 6 commits March 26, 2026 10:32

Merge branch 'main' into ci-144357

0db4d22

Share regex

e019a2f

Merge branch 'main' into ci-144357

0c5db07

Merge branch 'main' into ci-144357

d08d55b

Merge branch 'main' into ci-144357

89199ce

Merge branch 'main' into ci-144357

4a7db58

prdoyle approved these changes Mar 27, 2026

View reviewed changes

mamazzol merged commit 9156c15 into elastic:main Mar 27, 2026
36 checks passed

mamazzol added a commit to mamazzol/elasticsearch that referenced this pull request Mar 30, 2026

Change the approach to find the APM Agent property file in test (elas…

d877d5e

…tic#144928) # Conflicts: # muted-tests.yml

samxbr mentioned this pull request Apr 7, 2026

Unmute testGetReindexFollowsRelocation #145841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the approach to find the APM Agent property file in test#144928

Change the approach to find the APM Agent property file in test#144928
mamazzol merged 10 commits intoelastic:mainfrom
mamazzol:ci-144357

mamazzol commented Mar 25, 2026 •

edited

Loading

Uh oh!

elasticsearchmachine commented Mar 25, 2026

Uh oh!

coderabbitai bot commented Mar 25, 2026 •

edited

Loading

Reviews paused

Walkthrough

❌ Failed checks (1 inconclusive)

Uh oh!

prdoyle commented Mar 25, 2026

Uh oh!

mamazzol commented Mar 25, 2026

Uh oh!

mark-vieira left a comment

Uh oh!

mamazzol commented Mar 25, 2026

Uh oh!

prdoyle left a comment

Uh oh!

prdoyle Mar 25, 2026

Uh oh!

mamazzol Mar 25, 2026

Uh oh!

prdoyle Mar 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 25, 2026

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

❌ Failed checks (1 inconclusive)

Uh oh!

prdoyle commented Mar 25, 2026

Uh oh!

mamazzol commented Mar 25, 2026

Uh oh!

mark-vieira left a comment

Choose a reason for hiding this comment

Uh oh!

mamazzol commented Mar 25, 2026

Uh oh!

prdoyle left a comment

Choose a reason for hiding this comment

Uh oh!

prdoyle Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mamazzol Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

prdoyle Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mamazzol commented Mar 25, 2026 •

edited

Loading

coderabbitai bot commented Mar 25, 2026 •

edited

Loading