Skip to content

[APM] Add multi-shard search test case#80792

Merged
elasticsearchmachine merged 6 commits intoelastic:feature/apm-integrationfrom
DaveCTurner:2021-11-17-apm-search-demo
Nov 17, 2021
Merged

[APM] Add multi-shard search test case#80792
elasticsearchmachine merged 6 commits intoelastic:feature/apm-integrationfrom
DaveCTurner:2021-11-17-apm-search-demo

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

Adds a test case that creates and populates a couple of multi-shard
indices and executes a search against them.

Adds a test case that creates and populates a couple of multi-shard
indices and executes a search against them.
@DaveCTurner DaveCTurner requested a review from tlrx November 17, 2021 12:33
@DaveCTurner DaveCTurner added :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >test Issues or PRs that are addressing/adding tests labels Nov 17, 2021
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team. label Nov 17, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +147 to +156
for (SpanData capturedSpan : capturedSpans) {
logger.info("--> captured span [{}]", capturedSpan);
final String spanName = capturedSpan.getName();
if (spanName.equals(SearchAction.NAME)) {
gotSearchSpan = true;
}
if (spanName.equals(SearchTransportService.QUERY_CAN_MATCH_NODE_NAME)) {
gotCanMatchSpan = true;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: This could be simplified to: APMTracer.CAPTURING_SPAN_EXPORTER.findSpanByName(..) once https://github.com/elastic/elasticsearch/pull/80758/files is merged

.execute()
.actionGet(10, TimeUnit.SECONDS);

assertBusy(() -> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have all the spans captured by the time the search is complete?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes you're right, we're only using the BatchSpanProcessor for pushing them over the wire. (I extracted this PR from a branch in which I'm using the BatchSpanProcessor for everything).

Copy link
Copy Markdown
Member

@tlrx tlrx Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the APMTracer uses a BatchSpanProcessor which queues and asynchronously exports spans.

Sorry for the noise, I just saw the flushes() so it should be flushed at the time the test completes.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 17, 2021
@elasticsearchmachine elasticsearchmachine merged commit a23bf66 into elastic:feature/apm-integration Nov 17, 2021
@DaveCTurner DaveCTurner deleted the 2021-11-17-apm-search-demo branch November 17, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants