Group sending of datapoints #3

rafaeling · 2024-09-19T08:23:46Z

This PR implements the group sending of datapoints by a newly defined json file.

Databroker-perf creates two new gRPC channels for each group: one for the provider and one for the subscriber.
Each provider will update its group signal values to the Databroker at the cycle time specified (in milliseconds) in the JSON configuration file provided.
config_group.json file:

{
  "groups": [
    {
      "group_name": "Frame 1",
      "cycle_time_ms": 10,
      "signals": [
        {
          "path": "Vehicle.Speed"
        }
      ]
    },
    {
      "group_name": "Frame 2",
      "cycle_time_ms": 20,
      "signals": [
        {
          "path": "Vehicle.IsBrokenDown"
        },
        {
          "path": "Vehicle.IsMoving"
        },
        {
          "path": "Vehicle.AverageSpeed"
        }
      ]
    }
  ]
}

Run seconds test parameter implemented instead of iterations parameter since all groups should end at the same exact time.
--detail-output flag added to extend the test result view.
Max number of groups defined to 10.
Check docu for more info: https://github.com/eclipse-kuksa/kuksa-perf/blob/539d48951f5860bf34f87e87da66d450e0e6c46f/README.md

…perf to kuksa-perf

mikehaller

LGTM, some minor hints for potential improvement, but nothing critical.

mikehaller · 2024-09-19T09:32:20Z

README.md

+Group: Frame 1 | Cycle time(ms): 10
+ API: KuksaValV2
+ Elapsed time: 9.99 s
+ Rate limit: 10 ms between iterations
 Sent: 1000 iterations * 1 signals = 1000 updates


--help shows that "iterations" have been removed and replaced by "seconds"?

mikehaller · 2024-09-19T09:38:30Z

README.md

+
+By default, the results output will be shown like this:
+```
+[00:00:08] Group: Frame A | Cycle(ms): 10 | Current latency: 0.725 ms [==============================================================================================================] 8/8 seconds


Splitting the output results into groups as well seems counter-intuitive, harder to understand.

Now, the user has to do the mental work to combine and aggregate all the results of the individual groups into a single result. How would he do that? Calculating the average of all the averages?

Imho, sending signals is grouped (as seen by the provider, sort of CAN-Bus point of view), but measuring the latency is done on the application side (subscriber, which is on VSS level) and the subscriber doesn't really know about groups of CAN signals and cycle times.

May be a future feature request to also show the "Total summary over all groups".

Splitting the output results into groups as well seems counter-intuitive, harder to understand.

I agree.

Now, the user has to do the mental work to combine and aggregate all the results of the individual groups into a single result. How would he do that? Calculating the average of all the averages?

Exactly, and I don't think it's even possible to correctly calculate the average after the fact (without having access to the underlying data).

This should be done by the application.

mikehaller · 2024-09-19T09:43:10Z

configs/config_group_1.json

@@ -0,0 +1,13 @@
+{


Not sure why the change in config file format was necessary, probably due to giving them a name.

Without the need for a name, the grouping could have been derived from previous file format. There was already an object for the definition of a signal, and it would have been easier to add it there.

Example (based config3.json)

{ "signals": [ { "path": "Vehicle.Speed", "cycle_time": "10ms" }, { "path": "Vehicle.TraveledDistance" "cycle_time": "1000ms" }, { "path": "Vehicle.TraveledDistanceSinceStart" "cycle_time": "1000ms" } ] }

This is just a hint, because the input comes from DBC, which is defined signal-by-signal. When you want to convert from that input source into the new databroker-perf config file format, you have to implement the "grouping" logic again.

I understand the theoretical idea of having group names though, but there is no such thing as a "group name" for all signals of the same cycle time in DBC. User would have to come up with an artificial name like All signals with 10ms cycle time, which I doubt has any benefit.

mikehaller · 2024-09-19T09:44:24Z

src/main.rs

@@ -54,23 +51,23 @@ struct Args {
 #[clap(long, display_order = 4, default_value_t = 55555)]
 port: u64,

- /// Number of iterations to run (skip) before measuring the latency.
+ /// Seconds to run (skip) before measuring the latency.
 #[clap(
 long,
 display_order = 5,
 value_name = "ITERATIONS",


Why is this still called ITERATIONS if everything is switched to Seconds? Overlooked?

mikehaller · 2024-09-19T09:45:57Z

src/main.rs

- let endpoint = tonic::transport::Channel::from_shared(databroker_address.clone())
- .with_context(|| "Failed to parse server url")?;
- let endpoint = endpoint
- .initial_stream_window_size(1000 * 3 * 128 * 1024) // 20 MB stream window size


Wouldn't it be useful to have these endpoint settings configurable by the user with additional command line options?

We already know that you might want to fine-tune these settings depending on the available memory on the device and the expected bus load.

Could be separate feature/story, just a hint for the future.

mikehaller · 2024-09-19T09:48:25Z

src/measure.rs

+ let endpoint = tonic::transport::Channel::from_shared(databroker_address.clone())
+ .with_context(|| "Failed to parse server url")?;
+ let endpoint = endpoint
+ .initial_stream_window_size(1000 * 3 * 128 * 1024) // 20 MB stream window size


Comments mismatch with actual values (actual 393MB, but comment says 20 MB)
Imho, this should be changed.

It's very unrealistic to have so much memory on a target embedded device.

mikehaller · 2024-09-19T09:51:06Z

src/measure.rs

+ let endpoint = endpoint
+ .initial_stream_window_size(1000 * 3 * 128 * 1024) // 20 MB stream window size
+ .initial_connection_window_size(1000 * 3 * 128 * 1024) // 20 MB connection window size
+ .keep_alive_timeout(Duration::from_secs(1)) // 60 seconds keepalive time


Duplicate call to keep_alive_timeout()
Comment mismatches actual value.

Are these settings different than the defaults?

Just out of curiosity, if they differ and are more suitable, it would make sense to document it for real clients (not only for databroker-perf).

I'm not sure we want these customization here at all... And if we do, it should be clearly documented why they should be different from the defaults. Or add them as command line arguments in a future version, so they can be experimented with?

Just doing a quick run, the latency is 20% better if I remove these non-default settings...

argerus · 2024-09-19T11:36:25Z

README.md

+
+By default, the results output will be shown like this:
+```
+[00:00:08] Group: Frame A | Cycle(ms): 10 | Current latency: 0.725 ms [==============================================================================================================] 8/8 seconds


Splitting the output results into groups as well seems counter-intuitive, harder to understand.

I agree.

Now, the user has to do the mental work to combine and aggregate all the results of the individual groups into a single result. How would he do that? Calculating the average of all the averages?

Exactly, and I don't think it's even possible to correctly calculate the average after the fact (without having access to the underlying data).

This should be done by the application.

argerus · 2024-09-19T13:59:00Z

src/measure.rs

+ let endpoint = endpoint
+ .initial_stream_window_size(1000 * 3 * 128 * 1024) // 20 MB stream window size
+ .initial_connection_window_size(1000 * 3 * 128 * 1024) // 20 MB connection window size
+ .keep_alive_timeout(Duration::from_secs(1)) // 60 seconds keepalive time


I'm not sure we want these customization here at all... And if we do, it should be clearly documented why they should be different from the defaults. Or add them as command line arguments in a future version, so they can be experimented with?

Just doing a quick run, the latency is 20% better if I remove these non-default settings...

argerus · 2024-09-19T14:02:13Z

src/measure.rs


+ print!("\n\nSummary:");


I think it's better if all output goes through Term::stdout() as done in fn write_output(...), instead of using print!

argerus · 2024-09-19T14:09:05Z

src/main.rs

 #[clap(
 long,
 short,
 display_order = 1,
- default_value_t = 1000,
+ default_value_t = 8,


I think a better default would be be to run forever (i.e. have no default value for this option).

argerus · 2024-09-19T14:59:28Z

src/utils.rs

 };

-pub fn read_config(config_file: Option<&String>) -> Result<Vec<Signal>> {
+const MAX_NUMBER_OF_GROUPS: usize = 10;


Why do we want a limit here?

lukasmittag and others added 6 commits September 19, 2024 10:05

Use paths again to subscribe -> id changed in v2 & Rename databroker-…

bfb8a5d

…perf to kuksa-perf

Revert renaming to databroker-perf

4de5a70

Group sending of datapoints

f32d958

Use seconds run concept instead of iterations

95647a3

Update docu

5684b48

Update docu usage

539d489

mikehaller reviewed Sep 19, 2024

View reviewed changes

argerus requested changes Sep 19, 2024

View reviewed changes

argerus reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group sending of datapoints #3

Group sending of datapoints #3

rafaeling commented Sep 19, 2024 •

edited

Loading

mikehaller left a comment

mikehaller Sep 19, 2024

mikehaller Sep 19, 2024

argerus Sep 19, 2024

mikehaller Sep 19, 2024

mikehaller Sep 19, 2024

mikehaller Sep 19, 2024

mikehaller Sep 19, 2024

mikehaller Sep 19, 2024

argerus Sep 19, 2024

argerus Sep 19, 2024

argerus Sep 19, 2024

argerus Sep 19, 2024

argerus Sep 19, 2024

argerus Sep 19, 2024

Group sending of datapoints #3

Are you sure you want to change the base?

Group sending of datapoints #3

Conversation

rafaeling commented Sep 19, 2024 • edited Loading

mikehaller 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaeling commented Sep 19, 2024 •

edited

Loading