Fetch RuntimeInformation of `SERVICE` queries #1798

UNEXENU · 2025-02-12T13:35:15Z

Adds the retrieval of runtime information for the service query using a WebSocket client.

UNEXENU · 2025-02-12T13:39:38Z

@joka921 This is a first working poc, i will take car of test coverage and outdated comments in the next iteration.

codecov · 2025-02-12T16:24:19Z

Codecov Report

Attention: Patch coverage is 40.59829% with 139 lines in your changes missing coverage. Please review.

Project coverage is 89.87%. Comparing base (8fe0642) to head (45d827c).

Files with missing lines	Patch %	Lines
src/util/http/websocket/WebSocketClient.cpp	0.00%	121 Missing ⚠️
src/util/http/websocket/WebSocketClient.h	0.00%	9 Missing ⚠️
src/engine/Service.cpp	72.72%	3 Missing ⚠️
src/engine/Service.h	50.00%	2 Missing and 1 partial ⚠️
src/util/http/HttpClient.cpp	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1798      +/-   ##
==========================================
- Coverage   90.18%   89.87%   -0.31%     
==========================================
  Files         400      403       +3     
  Lines       38440    38661     +221     
  Branches     4306     4330      +24     
==========================================
+ Hits        34666    34746      +80     
- Misses       2479     2616     +137     
- Partials     1295     1299       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joka921

An initial look at everything but the tests,
I think currently this breaks cancellation, because I don't know whether the websocket connections will always close gracefully.

joka921 · 2025-02-12T20:40:56Z

src/engine/Operation.cpp

+
+// _____________________________________________________________________________
+cppcoro::generator<std::shared_ptr<RuntimeInformation>>
+Operation::getRuntimeInfoChildren() {
+  for (auto child : getChildren()) {
+    AD_CONTRACT_CHECK(child);
+    co_yield child->getRootOperation()->getRuntimeInfoPointer();
+  }
+}


We are currently porting back to C++17 , so we shouldn't use generators in new code anymore.
Why not simply use std::vector<RuntimeInformation*> or maybe even better absl::InlinedVector<RuntimeInformation*> as we almost never have more than 2 children.

joka921 · 2025-02-12T20:41:24Z

src/engine/Operation.cpp

-      AD_CONTRACT_CHECK(child);
-      _runtimeInfo->children_.push_back(
-          child->getRootOperation()->getRuntimeInfoPointer());
+    for (auto child : getRuntimeInfoChildren()) {


use references when you don't need or use the copies of the child (multiple occurences).

joka921 · 2025-02-12T20:42:01Z

src/engine/Operation.h

+  // Get access to the children's RuntimeInfo. Required for the `Service`, as
+  // it's children can't be accessed using `getChildren()` above.


Not required for the service but overridden by the Service.

joka921 · 2025-02-12T20:43:12Z

src/engine/RuntimeInformation.cpp

+  if (str == "fully materialized") {
+    return fullyMaterialized;
+  } else if (str == "lazily materialized") {
+    return lazilyMaterialized;
+  } else if (str == "in progress") {
+    return inProgress;
+  } else if (str == "not started") {
+    return notStarted;
+  } else if (str == "optimized out") {
+    return optimizedOut;
+  } else if (str == "failed") {
+    return failed;


Can you set up a static HashMap for this lookup, mostly for readability`.

joka921 · 2025-02-12T20:44:55Z

src/engine/RuntimeInformation.cpp

+  auto cacheStatusFromString = [](std::string_view str) {
+    using ad_utility::CacheStatus;
+    if (str == "cached_not_pinned") {
+      return CacheStatus::cachedNotPinned;
+    } else if (str == "cached_pinned") {
+      return CacheStatus::cachedPinned;
+    } else if (str == "computed") {
+      return CacheStatus::computed;
+    } else if (str == "not_in_cache_not_computed") {
+      return CacheStatus::notInCacheAndNotComputed;
+    } else {
+      throw std::runtime_error(
+          "Unknown string value was encountered in `fromString(CacheStatus)`");


maybe also a separate function + also use a lookup data structure.

And also note with the respective enums in a big comment, that these functions also have to be changed...

joka921 · 2025-02-13T08:30:11Z

src/util/http/HttpClient.h

+  // Upgrade the client to a websocket connection and return a generator of
+  // messages received from the server.
+  static cppcoro::generator<std::string> readWebSocketStream(
+      std::unique_ptr<HttpClientImpl> client, std::string_view host,
+      std::string_view target);
+


We are trying to get away from generators,
Please have a look at the InputRangeTypeErased etc. classes at the end of Iterators.h , and ask me if you have trouble.
And as stated above, you should have a possibility to cancel the websocket stream if the outer operation is cancelled.

joka921 · 2025-02-13T08:31:45Z

src/util/http/HttpClient.cpp

@@ -62,6 +62,14 @@ HttpClientImpl<StreamType>::HttpClientImpl(std::string_view host,
 // ____________________________________________________________________________
 template <typename StreamType>
 HttpClientImpl<StreamType>::~HttpClientImpl() {
+  // If the stream was upgraded to a websocket connection, try to close it.
+  if (ws_) {


This is something that you probably have to expose also manually (in a threadsafe way, but ask me for details if necessayr).

joka921 · 2025-02-13T08:32:12Z

src/util/http/HttpClient.cpp

@@ -102,6 +112,10 @@ HttpOrHttpsResponse HttpClientImpl<StreamType>::sendRequest(
  request.set(http::field::accept, acceptHeader);
  request.set(http::field::content_type, contentTypeHeader);
  request.set(http::field::content_length, std::to_string(requestBody.size()));
+  for (const auto& h : customHeaders) {


Suggested change

for (const auto& h : customHeaders) {

for (const auto& [key, value] : customHeaders) {

...

joka921 · 2025-02-13T08:37:56Z

src/util/http/HttpClient.cpp

+    client->ws_->read(buffer);
+    co_yield beast::buffers_to_string(buffer.data());
+    buffer.consume(buffer.size());
+  }


See here for something I found about cancellation

This should document, that even normal exits will throw an exception, so that calling code has to handle everything.

Or you use an explicit strand that you run in this thread etc, but I'll have to llok into that again.

joka921 · 2025-02-13T08:40:48Z

src/util/http/HttpClient.cpp

+// ____________________________________________________________________________
+cppcoro::generator<std::string> readHttpOrHttpsWebsocketStream(
+    const ad_utility::httpUtils::Url& url, std::string_view target) {
+  auto listen = [&]<typename Client>() -> cppcoro::generator<std::string> {


Have a look at the util/typeIdentity.h header, there i have a trick that makes the templated lambdas easier to call.

Signed-off-by: Johannes Kalmbach <[email protected]>

sparql-conformance · 2025-02-24T17:08:10Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=45d827c48d42553eb4073e687683446fe59fdd2e&prev=8fe06428ee1dddbb3ebcb41a1d93525075571bc1

sonarqubecloud · 2025-02-24T18:25:15Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

initial RuntimeInformation support for service queries

e4490ce

joka921 requested changes Feb 13, 2025

View reviewed changes

UNEXENU and others added 7 commits February 21, 2025 10:29

improved WebSocketClient

35cbcaa

Merge branch 'master' into service-fetch-rti

fc64268

merge fix

8851990

temp fix boost error

5c554d5

Playing around...

10f0ea2

Signed-off-by: Johannes Kalmbach <[email protected]>

Merge branch 'ad-freiburg:master' into service-fetch-rti

d531351

fixed missing path-concatenation

45d827c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch RuntimeInformation of `SERVICE` queries #1798

Fetch RuntimeInformation of `SERVICE` queries #1798

UNEXENU commented Feb 12, 2025

UNEXENU commented Feb 12, 2025

codecov bot commented Feb 12, 2025 •

edited

Loading

joka921 left a comment

joka921 Feb 12, 2025

joka921 Feb 12, 2025

joka921 Feb 12, 2025

joka921 Feb 12, 2025

joka921 Feb 12, 2025

joka921 Feb 12, 2025

joka921 Feb 13, 2025

joka921 Feb 13, 2025

joka921 Feb 13, 2025

joka921 Feb 13, 2025

joka921 Feb 13, 2025

joka921 Feb 13, 2025

sparql-conformance bot commented Feb 24, 2025

sonarqubecloud bot commented Feb 24, 2025

		// Get access to the children's RuntimeInfo. Required for the `Service`, as
		// it's children can't be accessed using `getChildren()` above.

	for (const auto& h : customHeaders) {
	for (const auto& [key, value] : customHeaders) {
	...

Fetch RuntimeInformation of SERVICE queries #1798

Are you sure you want to change the base?

Fetch RuntimeInformation of SERVICE queries #1798

Conversation

UNEXENU commented Feb 12, 2025

UNEXENU commented Feb 12, 2025

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparql-conformance bot commented Feb 24, 2025

Conformance check passed ✅

sonarqubecloud bot commented Feb 24, 2025

Quality Gate passed

Fetch RuntimeInformation of `SERVICE` queries #1798

Fetch RuntimeInformation of `SERVICE` queries #1798

codecov bot commented Feb 12, 2025 •

edited

Loading