Add project routing telemetry infrastructure - #155997
Conversation
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
ef4c9df to
79847fb
Compare
Adds the core data structures and recording hooks for CPS project routing usage telemetry in GET _cluster/stats: - ProjectRoutingUsageHolder: LongAdder-based counters for _search and ES|QL queries broken down by routing mode (alias_origin, alias_wildcard, custom_tags, named_expression) plus failure counters for recording failed CPS queries. - ProjectRoutingUsageSnapshot: Writeable point-in-time snapshot accumulated across nodes on the coordinator. - ClusterStatsResponse: emits separate top-level `tags` and `project_routing` blocks; `project_routing.queries` is the sum of search and esql totals computed at render time. - TransportSearchAction: records per-search telemetry gated on collectSearchTelemetry and hasLinkedProjects. - Stub infrastructure for Ticket 4: ClusterStatsTagsProvider, TagsConfigSnapshot, ActionPlugin extension point. JSON field names follow the PM spec: `queries`, `queries_project_routing`, `alias_origin`, `alias_wildcard`, `custom_tags`, `named_expression`, `in_SET`, `failures`. Tests cover: wire serialization, add() accumulation, toXContent suppression rules, failure-method no-ops and counter semantics, and ClusterStatsResponse assembly.
79847fb to
61213a7
Compare
| int total, | ||
| int totalCustom, | ||
| List<String> names, | ||
| int namedRoutingExpressionsTotal, |
There was a problem hiding this comment.
Do we need the totals here? Couldn't we just calculate them from the List?
There was a problem hiding this comment.
Good catch. Will be fixed in the next push.
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.field("total", total); | ||
| builder.field("total_custom", totalCustom); | ||
| builder.array("names", names.toArray(new String[0])); |
There was a problem hiding this comment.
I think XContentBuilder can handle lists? It has Iterable<?> support so I think it'd do the right thing?
There was a problem hiding this comment.
👍 Will be changed in the next push.
| CCSTelemetrySnapshot ccsTelemetrySnapshot, | ||
| CCSTelemetrySnapshot esqlTelemetrySnapshot | ||
| CCSTelemetrySnapshot esqlTelemetrySnapshot, | ||
| ProjectRoutingUsageSnapshot projectRoutingUsageSnapshot |
There was a problem hiding this comment.
Do we need to ensure non-null here? I think we are going the other route with tags... I am not a huge fan of nulls, but here I am wondering if we have nothing to display (which would be the case for all non-CPS cases I imagine) why create the object at all?
| searchCcsMetrics.writeTo(out); | ||
| esqlCcsMetrics.writeTo(out); | ||
| if (out.getTransportVersion().supports(PROJECT_ROUTING_USAGE_STATS)) { | ||
| projectRoutingUsageSnapshot.writeTo(out); |
There was a problem hiding this comment.
As per above, maybe optional object here would be better, for non-CPS cases?
| final CCSTelemetrySnapshot esqlMetrics; | ||
| final ProjectRoutingUsageSnapshot projectRoutingUsageSnapshot; | ||
| @Nullable | ||
| final TagsConfigSnapshot tagsConfig; |
There was a problem hiding this comment.
As noted above, here it's a bit inconsistent - one is nullable, the other is not. Is there a reason why? The seem to be both non-existant in non-CPS context, or am I missing something?
There was a problem hiding this comment.
I agree this is a little confusing. It is true that ProjectRoutingUsage and TagsConfig will only be present in serverless and both absent (in the _cluster/stats output) for stateful. The reason for the different handling here (one is null, the other is never null but can be full of zeros) is that:
tagsConfigis nullable because it comes from an optional plugin registration that may not exist at all (no serverless plugin = null provider). Null here means "feature not wired up". It gets wired up only on the serverless side.- The projectRoutingUsageSnapshot comes from ProjectRoutingUsageHolder, which is a built-in holder that always exists in UsageService on the es-core side. It always has a valid snapshot to return.
Neither will "render" in _cluster/stats for stateful. For projectRoutingUsageSnapshot, it will not be included in the _cluster/stats output unlesstotalQueries > 0 . (see: https://github.com/elastic/elasticsearch/pull/155997/changes#diff-e4a6036f7e574278a739adbb118f2ab0031e4c1abbe92bac125bb732c8011993R187)
| esqlQueriesTotal.increment(); | ||
| if (setClauseUsed) esqlWithSet.increment(); | ||
| if (info == null) return; | ||
| esqlWithProjectRouting.increment(); |
There was a problem hiding this comment.
This part seems to be copy-paste from the same part of the function above, maybe should be one function somehow?
|
|
||
| // ES|QL endpoint | ||
| private final LongAdder esqlQueriesTotal = new LongAdder(); | ||
| private final LongAdder esqlWithProjectRouting = new LongAdder(); |
There was a problem hiding this comment.
It feels like search and ESQL counters both have common substructure:
record RoutingCounters(LongAdder projectRouting, LongAdder aliasOrigin,
LongAdder aliasWildcard, LongAdder customTags,
LongAdder namedExpression, LongAdder failures);
There was a problem hiding this comment.
Good suggestion. Will be changed in the next push.
| @Nullable List<ProjectRoutingInfo> linkedProjects // null when CPS is disabled | ||
| @Nullable List<ProjectRoutingInfo> linkedProjects, // null when CPS is disabled | ||
| @Nullable ProjectRoutingRequestInfo projectRoutingRequestInfo, | ||
| boolean hasLinkedProjects |
There was a problem hiding this comment.
Not sure if it's avoidable but this boolean does't feel right for me. It feels like we should already have this information without it, but I can't yet figure out how to get rid of it. Maybe will need to think more about it, for now I am just recording I don't like it :)
There was a problem hiding this comment.
hasLinkedProjects is needed because linkedProjects reflects post-routing state and can't serve as the gate. Specifically, _alias:_origin (probably the most common routing expression) causes the resolver to set linkedProjects to empty (route to origin only), even when the project has configured linked projects; using linkedProjects.isEmpty() would silently suppress all counters for that case. hasLinkedProjects captures the pre-routing truth.
I added javadoc to cover the reasoning as well.
| * @param usedAliasOrigin true when the expression was exactly {@code _alias:_origin} | ||
| */ | ||
| public record ProjectRoutingRequestInfo( | ||
| List<String> tagsUsedInRouting, |
There was a problem hiding this comment.
This as I understand is supposed to keep all tags, but in fact we're just interested in one question - were there any custom tags among them? So maybe it should be just boolean usedCustomTags instead? Or we're going to use more stuff from here in the future?
There was a problem hiding this comment.
So maybe it should be just boolean usedCustomTags instead? Or we're going to use more stuff from here in the future?
Yes, right now it is just used to compute whether custom tags were used. So we don't need the list, a boolean would do, but the list does future-proof against new requirements, but perhaps that's unnecessary abstraction? I can make the change.
…ats/TransportClusterStatsAction.java Co-authored-by: Stanislav Malyshev <smalyshev@users.noreply.github.com>
| this.searchUsageHolder = usageService.getSearchUsageHolder(); | ||
| this.ccsUsageHolder = usageService.getCcsUsageHolder(); | ||
| this.esqlUsageHolder = usageService.getEsqlUsageHolder(); | ||
| this.usageService = usageService; |
There was a problem hiding this comment.
Here's a bit of inconsistency - we store components of UsageService above and then also keep the whole thing. To be consistent, we probably want tagsProvider and getProjectRoutingUsageHolder individually? Or dispense with that pattern and keep only UsageService but now it's half-and-half.
There was a problem hiding this comment.
Fixed. I've refactored to extract all five things we need (searchUsageHolder, ccsUsageHolder, esqlUsageHolder, projectRoutingUsageHolder, and tagsProvider) as final fields in the constructor, and dropped the usageService field entirely. This was also made cleaner by switching ClusterStatsTagsProvider to SPI registration using loadSingletonServiceProvider (comment), which means it's fully resolved before TransportClusterStatsAction is constructed — no more lazy read needed.
smalyshev
left a comment
There was a problem hiding this comment.
A couple of nitpicks, overall LGTM.
| ProjectRoutingUsageSnapshot snapshot = new ProjectRoutingUsageSnapshot(5L, 0L, 0L, 0L, 0L, 0L, 0L, 3L, 0L, 0L, 0L, 0L, 0L, 0L, 0L); | ||
| ClusterStatsResponse response = buildResponse(snapshot, null); | ||
| String json = Strings.toString(response); | ||
| assertThat(json, containsString("\"project_routing\"")); |
There was a problem hiding this comment.
String-matching JSON feels a bit fragile. Can't we use some existing XContent methods to do the same?
There was a problem hiding this comment.
Good call out. Will be fixed in next push.
| * A snapshot of the project's tag configuration for inclusion in {@code GET _cluster/stats}. | ||
| * Populated by the serverless cross-project module (Ticket 4). Emits the static config fields | ||
| * ({@code total}, {@code total_custom}, {@code names}, {@code named_routing_expressions}) inside | ||
| * the top-level {@code tags} object. |
There was a problem hiding this comment.
This comment is slightly inaccurate - tags name is not produced by this class but by the enveloping code, but the comment implies it builds the whole object.
There was a problem hiding this comment.
Which raises an question - should we make it produce the whole object? I am fine either way but the comment needs to match what happens.
There was a problem hiding this comment.
Code comment updated in next push.
| 1L // esql: total=8, with_pr=4, alias_wildcard=2, in_SET=3, failures=1 | ||
| ); | ||
| String json = toJson(snap); | ||
| assertThat(json, not(containsString("\"search\""))); |
There was a problem hiding this comment.
May want to also fix JSON string matching here
Adds the core data structures and recording hooks for CPS project routing usage telemetry in GET _cluster/stats:
tagsandproject_routingblocks;project_routing.queriesis the sum of search and esql totals computed at render time.JSON field names follow the PM spec:
queries,queries_project_routing,alias_origin,alias_wildcard,custom_tags,named_expression,in_SET,failures.Tests cover: wire serialization, add() accumulation, toXContent suppression rules, failure-method no-ops and counter semantics, and ClusterStatsResponse assembly.