RSDK-11991: Historical module data

[lia-viam]

Implements historical module data for C++ modules, using the DataConsumer class

Most of this PR consists in implementing rudimentary BSON serialization, which is tested in the unit tests

[stuqdog]

Hmm... maybe it's just that I don't have any real BSON experience but on reviewing this, it's not really clear to me how the semantics actually work. Do we expect that a typical user would know how to construct the appropriate BSONBytes for a query? If so, disregard! If not, we should maybe spend some time thinking about how to make this a bit more intuitive.

[stuqdog]

Milliseconds seems maybe overly precise to me. It's a bit unwieldy to work with and I imagine the cases where a user will care about sub-second precision in a query are probably not too high?

Also: it's a little confusing that the time_point argument for the default query sets the time since, and is distinct from the time_back field. I could easily imagine someone seeing that time_back is the name of the field and so, e.g., create a time point with an arg of one hour.

[stuqdog]

Perhaps also it would be nice to create an override of default_query that doesn't ask for a time_point but sets a default of 24 hours ago, akin to what we do in the go SDK.

[lia-viam]

Good points! So default_query is kind of an implementation detail, maybe we could move it to a private header entirely. Users shouldn't actually call default_query, because the other method will do it for them, I just wanted to be able to unit test it.

I chose milliseconds just on the basis of the BSON spec for timestamp, which is in milliseconds, but you can do

using namespace std::chrono;

func_taking_ms(milliseconds{hours{5}});

but maybe this could be a remark in the header.

The use of time point vs duration also comes down to testing, because I can't reliably do unit tests on something that calls now() - duration.

I could easily imagine someone seeing that time_back is the name of the field and so, e.g., create a time point with an arg of one hour.

This is a situation where the C++ type system would prevent someone from doing that, but also based on this discussion I think I should just hide this helper function away somewhere

[stuqdog]

This is a situation where the C++ type system would prevent someone from doing that

So I agree that the type system won't let someone pass milliseconds as opposed to a time_point but I'm worried about the inversion logic of a time_point being "time since epoch", but the argument time_back being "time before now". So yeah, someone couldn't accidentally do

using namespace std:chrono;

auto hour = milliseconds(hours{1});
auto query = default_query("id", "resource", hour);

but I think they could accidentally do

auto query = default_query("id", "resource", {hour});

thinking that this will give them everything in the past hour but actually it's giving them everything since an hour past the epoch (if I'm mistaken and you can't curly brace construct a time_point that way, let me know!).

I'm certainly willing to be convinced that this is overly defensive thinking on my part, but also I think just hiding this helper away from users is probably the best solution.

[stuqdog]

Hmm... can we not use ViamClient's from_env method to get the DataClient within the constructor (similar to how we do in python) to avoid asking users to pass around the DataClient?

[lia-viam]

I think we could also provide DataConsumer::from_env, the difference would just be that this implies DataConsumer owns its DataClient which is probably a moot point in a GC language like Python

[stuqdog]

I think that's probably fine if the DataConsumer owns/is responsible for the DataClient? Maybe there's a C++ memory reason to not like that but from a code design perspective I don't have a problem with it, I'd hope that in general users kind of just ignore the internals of the DataConsumer which becomes easier if there's a constructor that uses env vars to avoid asking for anything.

[stuqdog]

generally this looks good to me, though I do think the default_query method should probably be made private.