Add concurrent requests #8

calebbourg · 2022-04-05T13:44:18Z

Description

This PR adds a -i, --int <INT> flag to the CLI for sending INT number of concurrent requests to Ambi at a time.

caleb@calebs-MBP ambi_mock_client % ./target/debug/ambi_mock_client --help                      
Ambi Mock Client 0.1.0
Rust Never Sleeps community (https://github.com/Jim-Hodapp-Coaching/)
This application emulates a real set of hardware sensors that can report on environmental conditions
such as temperature, pressure, humidity, etc.

USAGE:
    ambi_mock_client [OPTIONS]

OPTIONS:
    -d, --debug
            Turns verbose console debug output on

    -h, --help
            Print help information

    -i, --int <INT>
            [default: 1]

    -V, --version
            Print version information

caleb@calebs-MBP ambi_mock_client % ./target/debug/ambi_mock_client -i 15

cli: Cli { debug: false, int: 15 }

Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"22.4","humidity":"58.2","pressure":"941","dust_concentration":"999","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"34.0","humidity":"18.2","pressure":"992","dust_concentration":"327","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"23.4","humidity":"66.5","pressure":"1014","dust_concentration":"986","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"24.0","humidity":"41.3","pressure":"1054","dust_concentration":"709","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"34.2","humidity":"99.7","pressure":"906","dust_concentration":"52","air_purity":"Fresh Air"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"32.7","humidity":"41.4","pressure":"1026","dust_concentration":"598","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"27.4","humidity":"85.1","pressure":"947","dust_concentration":"793","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"28.0","humidity":"33.6","pressure":"936","dust_concentration":"622","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"34.0","humidity":"14.8","pressure":"1077","dust_concentration":"813","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"21.6","humidity":"24.7","pressure":"941","dust_concentration":"380","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"16.7","humidity":"52.6","pressure":"1075","dust_concentration":"593","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"31.6","humidity":"1.2","pressure":"999","dust_concentration":"530","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"25.8","humidity":"56.8","pressure":"1054","dust_concentration":"990","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"19.5","humidity":"78.9","pressure":"931","dust_concentration":"729","air_purity":"High Pollution"}
Sending POST request to http://localhost:4000/api/readings/add as JSON: {"temperature":"27.5","humidity":"7.5","pressure":"1094","dust_concentration":"887","air_purity":"High Pollution"}
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"
Response from Ambi backend: "200"

…ion layout pattern.

… --debug CLI flag.

…e_debug_flag Split app logic and use debug flag for response output

src/lib.rs

glynos · 2022-04-05T18:26:26Z

src/lib.rs

        Err(e) => {
-            match cli.debug {
+            match debug {


Same comment as above, this can be an if statement instead of a pattern matching statement

Serneum

I can't find where it would be defined in the code (maybe part of the clap?) but can the description for -i be updated to include information on what purpose it serves in addition to the default value?

jhodapp · 2022-04-05T19:16:01Z

I can't find where it would be defined in the code (maybe part of the clap?) but can the description for -i be updated to include information on what purpose it serves in addition to the default value?

Indeed it can, just adding a comment above the element definition in the structure provides the printed help for this flag, e.g.: https://github.com/Jim-Hodapp-Coaching/ambi_mock_client/blob/master/src/lib.rs#L27

Serneum · 2022-04-05T20:02:03Z

Oh nice. I saw the comment above the one line and wondered if it was clever enough to do that. Thanks for the explanation

jhodapp · 2022-04-05T20:28:41Z

Absolutely, clap is an amazingly well designed library. Lots of strong interface design patterns to learn from it.

jhodapp

I'd love to see the addition of some kind of identifier to each thread that you spawn, so we can see in debug output which response corresponds to which thread.

For example, even a simple index or assigned thread id could be interesting and helpful.

I won't block merging this PR but consider it a strong preference to see it added, or I'd be ok with a follow-on PR.

calebbourg · 2022-04-06T14:32:41Z

@jhodapp @Serneum @glynos

Thank you all very much for your reviews. Learning a ton from the conversation.

I added a commit 59aa0e0

Which adds a few changes:

Adds a Output struct with a print() method and some custom output trait implementations. I thought this might be a good way to remove many of the if/else branching stuff while still maintaining the output we desire
The output struct contains a thread_id field that will display the ID of the current thread

I'd like to write some tests for the public methods I added but I wanted to give ya'll a chance to look this over and see if it's something we think can accomplish what we want. Mostly just hoping to zero in on a decent extendable interface for the Output struct (or can the idea if something better comes up)

calebbourg · 2022-04-06T14:33:30Z

src/lib.rs

+    }
+
+    fn print_to_stderr(&self) {
+      if self.debug {


This is how we accomplish debug output vs non-debug

calebbourg · 2022-04-06T14:33:55Z

src/lib.rs

+      }
+}
+
+impl fmt::Display for Output {


This trait is how we accomplish the customization for non-debug output

…printing

jhodapp

A wonderful contribution, thank you for this Caleb. I left several thoughts and ideas for how to evolve this even further. Take or leave whatever you want.

src/lib.rs

jhodapp · 2022-04-07T18:09:46Z

src/lib.rs

@@ -27,6 +28,10 @@ pub struct Cli {
    /// Turns verbose console debug output on
    #[clap(short, long)]
    pub debug: bool,
+
+    // Make int number of concurrent requests
+    #[clap(short, long, default_value_t = 1)]


Is there an upper limit to how many concurrent requests we want to allow for? Perhaps adding CLI interface data check on that would be in order. For example, do we really want to allow for 65k simultaneous threads, or at least without having tested it well? :)

jhodapp · 2022-04-07T18:19:54Z

src/lib.rs

+  }
+
+impl Output {
+    pub fn new(


I wonder if we could have one static instance of Output instead of having to call new() each time. That way it'd be a very lightweight operation both in execution time and in memory allocation. So it wouldn't be on the heap then, it'd be in a special section of memory called the Block Starting Symbol (BSS). These become global instances and are meant to be shared and used in different portions of an application.

Happy to discuss this more, just wanted to plant the idea.

jhodapp · 2022-04-07T18:21:45Z

src/lib.rs

@@ -56,6 +61,74 @@ impl Reading {
    }
 }

+#[derive(Debug)]
+struct Output {


One idea for you - what if we called this Log, or Logger or even more specific ResponseLog \ ResponseLogger instead of Output? There may even be naming convention that's common in other Rust programs here (and perhaps that is Output and I just aren't aware of this).

jhodapp · 2022-04-07T18:22:46Z

src/lib.rs

+    }
+
+    pub fn is_error(&self) -> bool {
+      self.error.is_some()


I absolutely love the use of Rust's Option concept here, fits perfectly in line with success vs error cases.

jhodapp · 2022-04-07T18:35:11Z

src/lib.rs

+        let handler = spawn(move ||
+            match send_request(URL, Client::new()) {
+              Ok(response) => {
+                Output::new(


In reference to my above suggestion for a different name and a static version, this then might look something like:

ResponseLog.print( None, Some(response), std::thread::current().id() );

The String could become implicit because it's a specific enough kind of logger type, and you could initialize the static instance of it with debug once instead of needing to pass it along every time.

One other thought: could the None and Some become a literal Option that you pass to print() in which it could figure out whether it's a success message vs an error message?

Just an idea, curious what you think.

Co-authored-by: Jim Hodapp <[email protected]>

…ing/ambi_mock_client into add_concurrent_requests

calebbourg · 2022-05-11T01:15:43Z

@jhodapp @glynos I added a commit 6c896df which adds aResponseLog struct that functions similar to how Jim described. It's not exactly the idea but this is all a work in progress as I'm learning more.

One interesting thing I've been finding is it's difficult to test anything that refers directly to types that are owned by external crates. My first instinct is to use dependency injection for example:

defining ResponseLog so that it holds an implementor of std::io::Write so that I can inject std::io::Stderr or std::io::Stdout depending on the context

#[derive(Debug, PartialEq)]
struct ResponseLog<'a, W: Write> {
    debug: bool,
    writer: &'a mut W,
}

and then pass a Vector in test to be able to prove that writing happens.

    fn response_log_prints() {
        let mut writer = Vec::new();
        let result = "123";
        let mut response_log = ResponseLog::new(false, &mut writer);
        let expected = result.as_bytes().to_vec();

        response_log.print(result);

        assert_eq!(writer, expected)
    }

It seems like the best thing to do would be to wrap any external dependencies in your own types and test that way but that's also a lot of code.

I'm curious how this is best handled in Rust. Specifically in testing.

jhodapp

Really great stuff Caleb, I think we're getting closer to this being a very strong and helpful contribution to the mock client! I left a few suggestions inline that I hope are helpful.

jhodapp · 2022-05-11T15:22:59Z

src/lib.rs

+}
+
+#[derive(Debug, PartialEq)]
+struct ResponseLog<'a, W: Write> {


This is great Caleb. Do you have a specific reason to not name this more broadly and only use it for logging responses? Could we use this as our logger more widely? Maybe you could share your thoughts on naming it Response specifically?

jhodapp · 2022-05-11T15:23:52Z

src/lib.rs

+}
+
+impl<'a, W: Write> ResponseLog<'a, W> {
+    pub fn new(debug: bool, writer: &'a mut W) -> Self {


Have you considered a static lifetime that is also trait bound? Here's an example of what I mean: https://doc.rust-lang.org/rust-by-example/scope/lifetime/static_lifetime.html#trait-bound

jhodapp · 2022-05-11T15:26:54Z

src/lib.rs

+                    Some(response),
+                    std::thread::current().id(),
+                );
+                ResponseLog::new(debug, &mut std::io::stdout()).print(result)


Whether the use of new() remains or you decide to take my advice and switch to a static instance of the logger, I think it makes ergonomic sense to separate instantiation of ResponseLog from using it print(). And then you can only create a single instance of it one time and not have to repeat instantiating it.

calebbourg self-assigned this Apr 5, 2022

calebbourg changed the base branch from master to use_debug_flag April 5, 2022 13:44

add -i flag for multiple concurrent requests

9d8aaa0

calebbourg force-pushed the add_concurrent_requests branch from 7971a5b to 9d8aaa0 Compare April 5, 2022 13:47

calebbourg requested review from jhodapp, Serneum and wumbabum April 5, 2022 13:47

jhodapp added 2 commits April 5, 2022 10:27

Move all application logic into src/lib.rs per standard Rust applicat…

584b6f2

…ion layout pattern.

Modifies HTTP request response debug printing to make use of the -d |…

61680eb

… --debug CLI flag.

jhodapp added the enhancement New feature or request label Apr 5, 2022

Merge pull request #9 from Jim-Hodapp-Coaching/split_app_logic_and_us…

5f46a75

…e_debug_flag Split app logic and use debug flag for response output

glynos reviewed Apr 5, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

glynos reviewed Apr 5, 2022

View reviewed changes

Serneum reviewed Apr 5, 2022

View reviewed changes

jhodapp reviewed Apr 5, 2022

View reviewed changes

Add Output struct for custom formatting

59aa0e0

calebbourg commented Apr 6, 2022

View reviewed changes

jhodapp and others added 3 commits April 6, 2022 20:16

Make use of the debug CLI flag for request response status and error …

09c7479

…printing

add -i flag for multiple concurrent requests

abe575e

Add Output struct for custom formatting

44dc890

jhodapp requested changes Apr 7, 2022

View reviewed changes

calebbourg and others added 4 commits April 7, 2022 16:58

Update description for -i flag

214ee76

Co-authored-by: Jim Hodapp <[email protected]>

use Output description in output

1857c0c

Merge branch 'add_concurrent_requests' of github.com:Jim-Hodapp-Coach…

b667689

…ing/ambi_mock_client into add_concurrent_requests

define local Error struct

2392f43

Add ResponseLog

6c896df

jhodapp requested changes May 11, 2022

View reviewed changes

AntoniosBarotsis mentioned this pull request Mar 2, 2023

Add CLI flags -n, -t, T and -p #15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concurrent requests #8

Add concurrent requests #8

calebbourg commented Apr 5, 2022

glynos Apr 5, 2022

Serneum left a comment

jhodapp commented Apr 5, 2022

Serneum commented Apr 5, 2022

jhodapp commented Apr 5, 2022

jhodapp left a comment •

edited

Loading

calebbourg commented Apr 6, 2022

calebbourg Apr 6, 2022

calebbourg Apr 6, 2022

jhodapp left a comment

jhodapp Apr 7, 2022

jhodapp Apr 7, 2022

jhodapp Apr 7, 2022

jhodapp Apr 7, 2022

jhodapp Apr 7, 2022

calebbourg commented May 11, 2022 •

edited

Loading

jhodapp left a comment

jhodapp May 11, 2022

jhodapp May 11, 2022

jhodapp May 11, 2022

Add concurrent requests #8

Are you sure you want to change the base?

Add concurrent requests #8

Conversation

calebbourg commented Apr 5, 2022

Description

Choose a reason for hiding this comment

Serneum left a comment

Choose a reason for hiding this comment

jhodapp commented Apr 5, 2022

Serneum commented Apr 5, 2022

jhodapp commented Apr 5, 2022

jhodapp left a comment • edited Loading

Choose a reason for hiding this comment

calebbourg commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhodapp 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

calebbourg commented May 11, 2022 • edited Loading

jhodapp 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

jhodapp left a comment •

edited

Loading

calebbourg commented May 11, 2022 •

edited

Loading