Allow Smaller PBF Responses #3507

kevinkreiser · 2022-01-12T02:48:02Z

NOTE: this pr is pointed at the other in progress PR #3506 once that is merged ill repoint this one at master

This PR tackles another one of the open items on the initial work we did to allow for for pbf interaction with the service namely this one:

Slim Responses: The Apiobject has lots of stuff the requester may not want, so we should allow for requesting only specific subfields to be returned (this will be the most efficient means of interaction).

The approach I took here was to add a small submessage to options proto which allows you to select which top level fields in the api protobuf message will be returned to the user. so if you are doing a route-like action you can say i just want directions to come back. this should make the response quite a lot slimmer. i'll get numbers on then shortly.

Ok I did a quick test with a medium sized route (Hummelstaedel to Altoona @dgearhart) and the size differences are as follows:

-rw-r--r--  1 kkreiser kkreiser 173678 Jan 13 10:50 pbf_fat
-rw-r--r--  1 kkreiser kkreiser  27897 Jan 13 10:51 json_output
-rw-r--r--  1 kkreiser kkreiser  20918 Jan 13 10:53 pbf_slim

So you'll see without slimming the pbf (ie keeping in the trip object) its about 625% larger than the json output, but when you slim it down to just the directions object you get a result that is 25% smaller and presumably way faster to parse.

kevinkreiser · 2022-01-12T02:48:28Z

CHANGELOG.md

   * CHANGED: Moved all protos to proto3 for internal request/response handling [#3457](https://github.com/valhalla/valhalla/pull/3457)
   * CHANGED: Allow up to 32 outgoing link edges on a node when reclassifying links [#3483](https://github.com/valhalla/valhalla/pull/3483)
   * CHANGED: Reuse sample::get implementation [#3471](https://github.com/valhalla/valhalla/pull/3471)
   * ADDED: Beta support for interacting with the http/bindings/library via serialized and pbf objects respectively [#3464](https://github.com/valhalla/valhalla/pull/3464)
   * CHANGED: Update xcode to 12.4.0 [#3492](https://github.com/valhalla/valhalla/pull/3492)
   * ADDED: Add JSON generator to conan [#3493](https://github.com/valhalla/valhalla/pull/3493)
+   * CHANGED: top_speed option: ignore live speed for speed based penalties [#3460](https://github.com/v     alhalla/valhalla/pull/3460)


this change log entry was out of order so i moved it down to where it belongs

kevinkreiser · 2022-01-12T02:48:57Z

docs/api/protocol-buffers.md

@@ -23,7 +23,7 @@ To use protobuf as a request/input to the HTTP API you need to do two things:
 * Send the proper HTTP header to signal a protobuf payload. The header should be: `Content-Type: application/x-protobuf`
 * Send protocol buffer's serialized bytes as the body of the HTTP request

-The message we use for the entire transaction is the `Api` message, whose definition you can find [here](../../proto/api.proto). All of the request parameters should be filled out via the `Options` message attached to the `Api` message. Most importantly, you will want to set your `format` to `pbf` and your `action` to the relevant API you are calling (though the HTTP request path also provides the latter). The rest of the request options depend on which API you are calling. For more information about what and which options to set for a given API please read that APIs specific docs regarding its request options.
+The message we use for the entire transaction is the `Api` message, whose definition you can find [here](../../proto/api.proto). All of the request parameters should be filled out via the `Options` message attached to the `Api` message. Most importantly, you will want to set your `format` to `pbf` and your `action` to the relevant API you are calling (though the HTTP request path also provides the latter). The `options` object also contains a subobject named `pbf_field_selector` which can be used to turn on/off the top level fields in the response. For example, if you only want the `directions` part of the protobuf response to be present (much smaller payload) then turn on only that flag in the field selector. The rest of the request options depend on which API you are calling. For more information about what and which options to set for a given API please read that APIs specific docs regarding its request options.


just a quick blerb about how to select only the parts of the response you want to have returned to you rather than the whole thing (which is the default)

kevinkreiser · 2022-01-12T02:49:53Z

proto/info.proto

@@ -43,4 +43,5 @@ message Info {
  repeated Statistic statistics = 1;      // stats that we collect during request processing
  repeated CodedDescription errors = 2;   // errors that occured during request processing
  repeated CodedDescription warnings = 3; // warnings that occured during request processing
+  bool is_service = 4;                    // was this a service request/response rather than a direct call to the library


this just marks whether the request was being handled by the server or by a direct library call, which is important to protobuf serialization because the former needs to know some of the options to properly handle serialization so they cant be omitted at certain parts of the processing

kevinkreiser · 2022-01-12T02:50:37Z

proto/options.proto

+  bool options = 1;
+  bool trip = 2;       // /trace_attributes
+  bool directions = 3; // /route /trace_route /optimized_route /centroid
+  bool status = 4;     // /status


these are the parts of the response you can turn on and off. by default they are all on (ie if you omit setting this option at all in the larger options object.

kevinkreiser · 2022-01-12T02:51:18Z

src/tyr/route_serializer.cc

-      return request.SerializeAsString();
+      return serializePbf(request);


you'll see this in a couple places. basically because pbf serialization had to get a bit more complicated i needed to throw it into a separate function

kevinkreiser · 2022-01-12T02:52:33Z

src/tyr/serializers.cc

@@ -156,6 +156,45 @@ void openlr(const valhalla::Api& api, int route_index, rapidjson::writer_wrapper
  }
  writer.end_array();
 }
+
+std::string serializePbf(Api& request) {


this is the new function for serailizing pbf. basically the only tricky part is, if its a service request, we need to keep the options bits attached to the object until we are done processing the request, so we swap it out before we serialize rather than actually clearing it and then we swap it back in after we have the bytes we want to return.

kevinkreiser · 2022-01-12T02:54:19Z

src/worker.cc

@@ -1123,9 +1123,18 @@ std::string serialize_error(const valhalla_exception_t& exception, Api& request)
  err_stat->set_value(1);
  err_stat->set_type(count);

-  // pbf format output
+  // pbf format output, we only send back the info with errors in it


yeah so we also have to handle errors properly. in this case i think the best signal to whether or not there is an error is to send back nothing but the error info. it is true that its possible to send back partial results but i thought it was better to send back only the error. again we have the same trick for working around the difference between service requests and direct library calls regarding having the options object present after seralization even if it was not requested to be included in the response

agreed, partial results are tricky..

kevinkreiser · 2022-01-12T02:54:45Z

src/worker.cc

  api.Clear();
+  api.mutable_info()->set_is_service(true);


for the http service path we immedately mark the request as being handled by the http service

kevinkreiser · 2022-01-12T03:05:51Z

test/gurka/test_pbf_api.cc

      EXPECT_EQ(actual_pbf.trip().SerializeAsString(), expected_pbf.trip().SerializeAsString());
+      EXPECT_TRUE(actual_pbf.has_options());


first i extended the existing testing to check that in the default case all the fields come back, then i made sure that the setting of the field selector works by selecting only the trip part and checking that only it came back (except info which is non optional and except for the /status service which doesnt have a trip associated with it)

kevinkreiser · 2022-01-12T03:06:09Z

test/gurka/test_pbf_api.cc

@@ -91,6 +105,10 @@ TEST(pbf_api, pbf_error) {
      auto pbf_bytes = serialize_error(e, api);
      Api actual;
      EXPECT_TRUE(actual.ParseFromString(pbf_bytes));
+      EXPECT_FALSE(actual.has_options());


here i just make sure that you only get back the info object with the error message and nothign else when an error occurs

kevinkreiser · 2022-01-12T03:06:56Z

valhalla/tyr/serializers.h

@@ -91,7 +91,7 @@ std::string serializeTransitAvailable(const Api& request,
 * @param results     The vector of trip paths and match results for each match found
 */
 std::string serializeTraceAttributes(
-    const Api& request,
+    Api& request,


because these serializers now call the new serializer below which modifies the api object, the const could no longer be used

* Not use live/predicted speeds for speed penalties * Drop only live speeds from top_speed penalties * Apply top speed changes for TaxiCost * remove unnecessary dynamicflowmask anymore * Test that live speed is disabled to top_speed option * Update CHANGELOG.md * Drop newly added flow constant, use bitwise negation inplace The compiler is expected to handle any constant operations so there's no need to bring new variables * Apply top_speed changes to other costings * Fix closure test: use common expected route for motorscooter * Refactor: extract speed based penalty calculation

…buf api

* Use roads under construction * updated lua code to include roads under construction; * added a new config option to include or exclude roads under construction from valhalla graph; * exclude roads under construction from routing; * exclude roads under construction from shortcuts building. * Added tests for roads under construction * Update changelog * Update taginfo.json * Add 'hov' costing to test constructions * Put 'construction' use above the transit uses * Turn off access for roads under construction * Remove needless 's' from the option 'include_constructions' * Turn off access for construction in lua; - exclude construction from density calculation; - do not use construction as an intersecting edge; * Exclude roads under construction from stopimpact calculation * Fix isochrone test * Update changelog

…buf api

kevinkreiser · 2022-01-12T23:33:48Z

CHANGELOG.md

@@ -47,15 +47,16 @@
   * ADDED: isochrone action for /expansion endpoint to track dijkstra expansion [#3215](https://github.com/valhalla/valhalla/pull/3215)
   * CHANGED: remove boost from dependencies and add conan as prep for #3346 [#3459](https://github.com/valhalla/valhalla/pull/3459)
   * CHANGED: Remove boost.program_options in favor of cxxopts header-only lib and use conan to install header-only boost. [#3346](https://github.com/valhalla/valhalla/pull/3346)
-   * CHANGED: top_speed option: ignore live speed for speed based penalties [#3460)(https://github.com/valhalla/valhalla/pull/3460)


this was at the wrong spot so i moved it to where it lands chronologically, also there was a typo in it

…e format string

kevinkreiser · 2022-01-13T16:16:03Z

src/proto_conversions.cc

@@ -265,6 +265,7 @@ bool Options_Format_Enum_Parse(const std::string& format, Options::Format* f) {
      {"json", Options::json},
      {"gpx", Options::gpx},
      {"osrm", Options::osrm},
+      {"pbf", Options::pbf},


missed this bug in a previous pr. the result of not having this is that if you send a json request but you want a pbf response it would still send you json. pbf request with pbf response worked fine though.

is that worth a small test? (didn't check, maybe there is one now?)

…ather than duplicating

nilsnolde

lgtm (well, apart from tiny fail on osx)

only thing I'm wondering: couldn't we infer from the action which pbf objects we return without the user needing to specify it? is it a clean mapping of action -> PBF object? e.g. I guess there's nothing useful in status or directions if it's a /trace_attributes call right?

EDIT: if that's the case, how about an auto member in the PbfFieldSelector (if possible defaulting to true)? then you'd still have the chance to get the entire thing for whatever reason, but opt-in instead of opt-out?

nilsnolde · 2022-01-14T09:32:11Z

src/worker.cc

@@ -1123,9 +1123,18 @@ std::string serialize_error(const valhalla_exception_t& exception, Api& request)
  err_stat->set_value(1);
  err_stat->set_type(count);

-  // pbf format output
+  // pbf format output, we only send back the info with errors in it


agreed, partial results are tricky..

kevinkreiser · 2022-01-14T14:07:05Z

only thing I'm wondering: couldn't we infer from the action which pbf objects we return without the user needing to specify it

that was the other option that i considered but its very unflexible, here are some cases off the top of my head that you couldnt do if we did it that way:

what if you want to do a route but get back only the trip (ie all the metadata along the route only). that would not be possible by infering based on the action. yes you can get this type of information via trace_attributes, but map matching has big limitations compared to routing so you couldnt really get metadata for a route that way
another case you might be interested in is getting back the options object because it contains all the information about what edge candidates were found when doing the route
yet another option would be to do performance testing by getting back only the info object which contains the latencies for each part of the request

in any case i convinced myself that there was enough value in allowing the user to select it that i shouldnt select it for them. but i definitely did think about doing it the simplier way first

nilsnolde · 2022-01-14T14:24:25Z

I'd argue that the majority of PBF requests will only want the most obvious, i.e. similar result as json. that's why I think design-wise it might more sense to opt-in to full responses due to the incurred size penalty (similar to e.g. opt-in verbose flag for /locate & /status). right now we'd have to opt-out.

not necessarily saying it should be done in this PR, could easily do a backwards-compatible follow-up PR (I think), but an auto approach defaulting to true while all others default to false could work no? if auto then infer from action, if any of the fields which are not inferred by the action are set, include those as well. if not you'd only get the inferred objects back. if auto is false and none of the others is set to true, raise (or silently put auto: true)?

kevinkreiser · 2022-01-14T14:31:09Z

@nilsnolde its all beta at the moment so we dont need to worry about compatiblity at this point.

why dont we make the defaults the obvious thing then route -> directions and so forth and let people opt in to more or less if they want it? would that work for you? im happy to add that to this pr, its a tiny change

nilsnolde · 2022-01-14T14:33:35Z

right, same thing.. I'd favor that, if it's cool with you too, perfect, thanks:)

…ate tests to prove it

kevinkreiser · 2022-01-14T16:49:47Z

CHANGELOG.md

   * ADDED: Add `include_construction` option into the config to include/exclude roads under construction from the graph [#3455](https://github.com/valhalla/valhalla/pull/3455)
   * CHANGED: Refactor options protobuf for Location and Costing objects [#3506](https://github.com/valhalla/valhalla/pull/3506)
+   * ADDED: New options to control what fields of the pbf are returned when pbf format responses are requested [#3207](https://github.com/valhalla/valhalla/pull/3507)


bad merge will fix

kevinkreiser added 6 commits January 6, 2022 22:56

sequester outputs of location correlation in pbf

c472855

missed a spot

597ddac

refactor costing protos

e8b699a

lint and docs

d317def

Merge remote-tracking branch 'origin/master' into kk_pbf_refactor

5897cab

fix changelog

cd9c0aa

kevinkreiser requested review from nilsnolde and dnesbitt61 January 12, 2022 02:48

kevinkreiser commented Jan 12, 2022

View reviewed changes

kevinkreiser force-pushed the kk_slim_pbf branch from 429a574 to aa45d47 Compare January 12, 2022 03:04

kevinkreiser commented Jan 12, 2022

View reviewed changes

kevinkreiser force-pushed the kk_slim_pbf branch from aa45d47 to 0e84088 Compare January 12, 2022 15:24

merkispavel and others added 3 commits January 12, 2022 16:44

allow the caller to ask for only certain parts of the top level proto…

ea6a9f5

…buf api

Base automatically changed from kk_pbf_refactor to master January 12, 2022 23:14

kevinkreiser changed the base branch from master to 1lessalgo January 12, 2022 23:15

kevinkreiser changed the base branch from 1lessalgo to master January 12, 2022 23:15

kevinkreiser force-pushed the kk_slim_pbf branch from a30e400 to 9f97830 Compare January 12, 2022 23:23

kevinkreiser changed the base branch from master to 1lessalgo January 12, 2022 23:23

kevinkreiser changed the base branch from 1lessalgo to master January 12, 2022 23:23

kevinkreiser changed the base branch from master to cleanup January 12, 2022 23:24

kevinkreiser changed the base branch from cleanup to master January 12, 2022 23:24

allow the caller to ask for only certain parts of the top level proto…

6469433

…buf api

kevinkreiser force-pushed the kk_slim_pbf branch from 9f97830 to 6469433 Compare January 12, 2022 23:31

kevinkreiser commented Jan 12, 2022

View reviewed changes

kevinkreiser requested a review from purew January 13, 2022 14:37

kevinkreiser added 2 commits January 13, 2022 10:37

Merge remote-tracking branch 'origin/kk_slim_pbf' into kk_slim_pbf

4db7a0f

if you want pbf and you are sending json as input we need to parse th…

478b95d

…e format string

kevinkreiser commented Jan 13, 2022

View reviewed changes

kevinkreiser added 3 commits January 13, 2022 11:17

changelog merge again

b406d25

changelog merge again

24eacb0

have the main parsing function do the request clearing in one place r…

d8d5c0b

…ather than duplicating

nilsnolde previously approved these changes Jan 14, 2022

View reviewed changes

kevinkreiser added 2 commits January 14, 2022 09:34

Merge remote-tracking branch 'origin/master' into kk_slim_pbf

fe15d4c

when not specified default to minimal pbf return for pbf formats. upd…

caf74b7

…ate tests to prove it

kevinkreiser dismissed nilsnolde’s stale review via caf74b7 January 14, 2022 15:16

kevinkreiser added 2 commits January 14, 2022 10:19

kick ci

e5905cf

fix mac build

e53b7b5

kevinkreiser commented Jan 14, 2022

View reviewed changes

nilsnolde self-requested a review January 14, 2022 16:50

kevinkreiser added 2 commits January 14, 2022 11:50

changelog formatting

f61aa9c

changelog formatting

57df8cc

nilsnolde approved these changes Jan 14, 2022

View reviewed changes

kevinkreiser merged commit a670418 into master Jan 14, 2022

kevinkreiser deleted the kk_slim_pbf branch January 14, 2022 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Smaller PBF Responses #3507

Allow Smaller PBF Responses #3507

kevinkreiser commented Jan 12, 2022 •

edited

Loading

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

nilsnolde Jan 14, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022

kevinkreiser Jan 12, 2022 •

edited

Loading

kevinkreiser Jan 13, 2022

nilsnolde Jan 14, 2022

nilsnolde left a comment •

edited

Loading

nilsnolde Jan 14, 2022

kevinkreiser commented Jan 14, 2022

nilsnolde commented Jan 14, 2022

kevinkreiser commented Jan 14, 2022

nilsnolde commented Jan 14, 2022

kevinkreiser Jan 14, 2022

		return request.SerializeAsString();
		return serializePbf(request);

		EXPECT_EQ(actual_pbf.trip().SerializeAsString(), expected_pbf.trip().SerializeAsString());
		EXPECT_TRUE(actual_pbf.has_options());

Allow Smaller PBF Responses #3507

Allow Smaller PBF Responses #3507

Conversation

kevinkreiser commented Jan 12, 2022 • edited Loading

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

kevinkreiser Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Jan 14, 2022

nilsnolde commented Jan 14, 2022

kevinkreiser commented Jan 14, 2022

nilsnolde commented Jan 14, 2022

Choose a reason for hiding this comment

kevinkreiser commented Jan 12, 2022 •

edited

Loading

kevinkreiser Jan 12, 2022 •

edited

Loading

nilsnolde left a comment •

edited

Loading