CCS metadata is opt-in in ESQL JSON responses #114437

quux00 · 2024-10-09T17:40:14Z

Since Kibana only needs CCS metadata in ESQL responses from certain well-defined locations,
we are making CCS metadata opt-in. This feature is be patterned after ESQL profiling, where
you specify "profile": true in the ESQL body and if you asked for it will be present in the response
always (it will be written to the .async-search index and you can’t turn it off in later async-search
requests against this particular query ID) and if you didn’t ask for it at the beginning it will never
be present (it will NOT be written to the .async-search index when it is persisted).

The new option is "include_ccs_metadata": true/false.

github-actions · 2024-10-09T17:40:25Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-10-09T17:40:38Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

build-tools-internal/src/main/resources/forbidden/jdk-deprecated.txt

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlExecutionInfo.java

smalyshev · 2024-10-09T18:06:38Z

Couple of nitpicks, otherwise LGTM.

…a-optin

quux00 · 2024-10-09T19:02:39Z

When I add a new ESQL option/flag like include_ccs_metadata do I need to update the capabilities or features list?

And for that matter should the CCS metadata feature in general (added in earlier PR) be listed as either a capability or a feature in those classes?

…etadata to MultiClustersIT and CrossClustersQueryIT

…a-optin

astefan

In EsqlMediaTypeParser we make a validation that considers the a combination of format and the columnar parameter value in the sense that columnar doesn't make sense to be used with any format. I think include_ccs_metadata parameter is of the same type: it can only be used for json format and I think we need a similar check.

astefan · 2024-10-10T12:33:07Z

...r/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClustersIT.java

-        Map<String, Object> resp = runEsql(new RestEsqlTestCase.RequestObjectBuilder().query(query).build());
+    private Map<String, Object> run(String query, boolean includeCCSMetadata) throws IOException {
+        Map<String, Object> resp = runEsql(
+            new RestEsqlTestCase.RequestObjectBuilder().query(query).includeCCSMetadata(includeCCSMetadata).build()


Using include_ccs_metadata only on this IT test suite is not enough I think.
Users can use include_ccs_metadata on any kind of REST request (multi cluster, single cluster single node, multi-node, mixed version, json/txt/tsv formats). I would, also, double check that "columnar": true (see this one in docs) still works with this new addition to the JSON response.

I would, also, double check that "columnar": true still works with this new addition to the JSON response.

It does. I tested it manually and I have added a test to MultiClustersIT that uses columnar format with include_ccs_metadata=true and it is passing.

I think include_ccs_metadata parameter is of the same type: it can only be used for json format and I think we need a similar check

OK, I can add this restriction. If later we want to support it with SMILE, YAML and CBOR we can relax it.

I have added a restriction that include_ccs_metadata can only be used with format=JSON. Tests have been added / updated to match this restriction.

…Metadata and ensure correctly set on the EsqlExecutionInfo

…a-optin

…mat=JSON. Tests have been added / updated to match this restriction.

…a-optin

...l/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersQueryIT.java

nik9000 · 2024-10-10T17:53:21Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlMediaTypeParser.java

+    }
+
+    private static void validateIncludeCCSMetadata(boolean includeCCSMetadata, MediaType fromMediaType) {
+        if (includeCCSMetadata && fromMediaType != XContentType.JSON) {


I think it's ok so long as the media type isn't text like the instanceof above. I expect kibana will use one of the binary ones at some point.

OK, in the latest commit I've changed it to be like columnar and work with any XContent type.

…not text formats

…a-optin

astefan

LGTM with one small addition, if you think it's worth it:

in x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/RestEsqlIT.java if you can add one test that doesn't use the ccs remote cluster format for indices. You could also simply adjust testBasicEsql() test method to receive a random value for include_ccs_metadata. This is to test a simple query like from test_index (no CCS involved) that behaves normally in the presence of include_ccs_metadata.

Thanks, @quux00 👍

astefan · 2024-10-11T07:10:38Z

...r/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClustersIT.java

+        Map<String, Object> resp = runEsql(
+            new RestEsqlTestCase.RequestObjectBuilder().query(query).includeCCSMetadata(true).columnar(true).build()
+        );
+        logger.info("--> query {} response {}", query, resp);


No, I'm still using that method to have a test that does both columnar: true and includes_ccs_metadata:true.

…a-optin

quux00 · 2024-10-11T14:01:48Z

@elasticmachine run elasticsearch-ci/part-3

quux00 · 2024-10-11T14:08:40Z

@elasticmachine run Elasticsearch Serverless Checks

quux00 · 2024-10-11T14:14:03Z

buildkite test this

…a-optin

quux00 · 2024-10-11T17:49:35Z

@elasticmachine run elasticsearch-ci/rest-compatibility

quux00 · 2024-10-11T17:50:01Z

@elasticmachine run elasticsearch-ci/validate-changelogs

…a-optin

elasticsearchmachine · 2024-10-11T19:04:36Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 114437

Since Kibana only needs CCS metadata in ESQL responses from certain well-defined locations, we are making CCS metadata opt-in. This feature is patterned after ESQL profiling, where you specify "profile": true in the ESQL body and if you asked for it will be present in the response always (it will be written to the .async-search index and you can’t turn it off in later async-search requests against this particular query ID) and if you didn’t ask for it at the beginning it will never be present (it will NOT be written to the .async-search index when it is persisted). The new option is "include_ccs_metadata": true/false.

quux00 · 2024-10-13T17:33:18Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

Since Kibana only needs CCS metadata in ESQL responses from certain well-defined locations, we are making CCS metadata opt-in. This feature is patterned after ESQL profiling, where you specify "profile": true in the ESQL body and if you asked for it will be present in the response always (it will be written to the .async-search index and you can’t turn it off in later async-search requests against this particular query ID) and if you didn’t ask for it at the beginning it will never be present (it will NOT be written to the .async-search index when it is persisted). The new option is "include_ccs_metadata": true/false. (cherry picked from commit fd9d733) # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

Since Kibana only needs CCS metadata in ESQL responses from certain well-defined locations, we are making CCS metadata opt-in. This feature is patterned after ESQL profiling, where you specify "profile": true in the ESQL body and if you asked for it will be present in the response always (it will be written to the .async-search index and you can’t turn it off in later async-search requests against this particular query ID) and if you didn’t ask for it at the beginning it will never be present (it will NOT be written to the .async-search index when it is persisted). The new option is "include_ccs_metadata": true/false. (cherry picked from commit fd9d733)

## Summary Retrieves the cluster details info on demand. Follow up of elastic/elasticsearch#114437 This will display the cluster details info only when needed: - Discover inspector - Lens ES|QL charts inspector

…6105) ## Summary Retrieves the cluster details info on demand. Follow up of elastic/elasticsearch#114437 This will display the cluster details info only when needed: - Discover inspector - Lens ES|QL charts inspector (cherry picked from commit fbe15fe)

) (#196511) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Retrieves ccs information for inspector on demand (#196105)](#196105)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Stratoula Kalafateli <[email protected]>

Since Kibana only needs CCS metadata in ESQL responses from certain well-defined locations, we are making CCS metadata opt-in. This feature is patterned after ESQL profiling, where you specify "profile": true in the ESQL body and if you asked for it will be present in the response always (it will be written to the .async-search index and you can’t turn it off in later async-search requests against this particular query ID) and if you didn’t ask for it at the beginning it will never be present (it will NOT be written to the .async-search index when it is persisted). The new option is "include_ccs_metadata": true/false.

quux00 added 3 commits October 9, 2024 12:59

ESQL CCS metadata in responses is opt-in, off by default

f627d45

Cleanup and updated end user API docs

30b65b4

Added includeCCSMetadata to EsqlExecutionInfo

9d619f4

quux00 added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.16.0 v9.0.0 labels Oct 9, 2024

quux00 requested review from nik9000 and smalyshev October 9, 2024 17:40

quux00 requested a review from a team as a code owner October 9, 2024 17:40

quux00 requested a review from astefan October 9, 2024 17:40

smalyshev reviewed Oct 9, 2024

View reviewed changes

build-tools-internal/src/main/resources/forbidden/jdk-deprecated.txt Show resolved Hide resolved

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlExecutionInfo.java Outdated Show resolved Hide resolved

quux00 added 4 commits October 9, 2024 14:25

Restored jdk-deprecated.txt

587500e

PR feedback: fixed code comment

3afa679

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

2cc8c38

…a-optin

Request.includeCCSMetadata passed into EsqlExecutionInfo ctor

03d37e5

quux00 added 2 commits October 9, 2024 17:08

Fixed failing MultiClustersIT. Adding randomization for include_ccs_m…

ae46e37

…etadata to MultiClustersIT and CrossClustersQueryIT

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

f487781

…a-optin

astefan reviewed Oct 10, 2024

View reviewed changes

quux00 added 4 commits October 10, 2024 09:19

Updated the CrossCluster IT tests to set random values for includeCCS…

3e6849d

…Metadata and ensure correctly set on the EsqlExecutionInfo

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

4cd2513

…a-optin

Added restriction that include_ccs_metadata can only be used with for…

6703610

…mat=JSON. Tests have been added / updated to match this restriction.

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

b0807b8

…a-optin

quux00 requested a review from astefan October 10, 2024 17:50

nik9000 reviewed Oct 10, 2024

View reviewed changes

quux00 added 2 commits October 10, 2024 15:17

PR feedback: include_ccs_metadata works with all XContent types, but …

6f7bb2a

…not text formats

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

ee53b6b

…a-optin

astefan approved these changes Oct 11, 2024

View reviewed changes

quux00 added 2 commits October 11, 2024 09:18

PR feedback: Added include_ccs_metadata into RestEsqlIT

6da4841

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

1191432

…a-optin

quux00 added 3 commits October 11, 2024 10:21

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

2d2dd09

…a-optin

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

f8ea2bf

…a-optin

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

e6d95bb

…a-optin

Merge remote-tracking branch 'elastic/main' into esql-ccs/ccs-metadat…

c35af29

…a-optin

quux00 merged commit fd9d733 into elastic:main Oct 11, 2024
16 checks passed

elasticsearchmachine added the backport pending label Oct 11, 2024

quux00 mentioned this pull request Oct 13, 2024

[8.x] CCS metadata is opt-in in ESQL JSON responses (#114437) #114688

Merged

stratoula mentioned this pull request Oct 14, 2024

[ES|QL] Retrieves ccs information for inspector on demand elastic/kibana#196105

Merged

CCS metadata is opt-in in ESQL JSON responses #114437

CCS metadata is opt-in in ESQL JSON responses #114437

Uh oh!

Conversation

quux00 commented Oct 9, 2024

Uh oh!

github-actions bot commented Oct 9, 2024

Uh oh!

elasticsearchmachine commented Oct 9, 2024

Uh oh!

Uh oh!

Uh oh!

smalyshev commented Oct 9, 2024

Uh oh!

quux00 commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

astefan Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

quux00 Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

quux00 Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nik9000 Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

quux00 Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

astefan Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

quux00 Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

quux00 commented Oct 11, 2024

Uh oh!

quux00 commented Oct 11, 2024

Uh oh!

quux00 commented Oct 11, 2024

Uh oh!

quux00 commented Oct 11, 2024

Uh oh!

quux00 commented Oct 11, 2024

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 11, 2024

💔 Backport failed

Uh oh!

quux00 commented Oct 13, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

quux00 commented Oct 9, 2024 •

edited

Loading