request counters #75

pwalski · 2024-03-15T09:41:00Z

Resolves: #56
Resolves: #57

…tors drop test. Yagna ver update.

…date.

pwalski · 2024-03-19T11:20:22Z

src/main.rs

            let mut proxy = GsbToHttpProxy {
-                // base_url: "http://10.30.13.8:7861/".to_string(),


Accidentally removed it. Sorry.

pwalski · 2024-03-19T11:21:35Z

src/main.rs

-            current_usage[idx] = duration.to_std()?.as_secs_f64();
-        }
+        let current_usage = counters.current_usage().await;
+        let timestamp = Utc::now().timestamp();


Maybe counters.current_usage() should return tuple of usage vector and timestamp 🤷‍♂️

nieznanysprawiciel

I appreciate efforts to make abstractions for Counters and Requests monitoring.I think that they could be better.

First of all some part of counters implementations should be in yagna. Requests counters are something that every user of the library would like to use. On the other side duration counter is something more general and should probably stay in ya-runtime-ai.
Abstractions that you build should reflect this split of responsibility.

The second thing: some parts of abstractions are over-complicated. I don't thing you need as many traits. I posted details in comments.

nieznanysprawiciel · 2024-03-19T16:22:12Z

src/counter/combined.rs

+    }
+}
+
+// Failsafe for not calling `on_response`.


nieznanysprawiciel · 2024-03-19T16:51:36Z

src/counter.rs

+#[derive(Debug)]
+enum SupportedCounter {
+    Duration(DurationCounter),
+    RequestsDuration(RequestsDurationCounter),
+    RequestsCount(RequestsCounter),
+}
+
+impl FromStr for SupportedCounter {
+    type Err = anyhow::Error;
+
+    fn from_str(counter: &str) -> anyhow::Result<Self, Self::Err> {
+        let counter = match counter {
+            "golem.usage.duration_sec" => SupportedCounter::Duration(Default::default()),
+            "golem.usage.gpu-sec" => SupportedCounter::RequestsDuration(Default::default()),
+            "ai-runtime.requests" => SupportedCounter::RequestsCount(Default::default()),
+            _ => bail!("Unsupported counter: {}", counter),
+        };
+        Ok(counter)
+    }
+}


I think this is not the best abstraction for counters we can have.
It serves well on creation, because number of possibilities is limited, but later you must implement every trait for this object. Imagine how much work would it cost to add new type of counter in comparison to just storing vector of boxed dyn traits.

It would be better to replace this with something like factory for counters.

nieznanysprawiciel · 2024-03-19T17:05:11Z

src/counter.rs

+trait RequestMonitoringCounter: Counter {
+    fn on_request(&mut self, request_time: DateTime<Utc>);
+    fn on_response(&mut self, response_time: DateTime<Utc>);
+}


Why do you need another trait here if you have ResponseMonitor and RequestMonitor already?

ResponseMonitor and RequestMonitor are splitted only to handle automatic on_response on drop of ResponseMonitor. Besides these traits do not accept time as a parameter. I pass time parameter because I want to use same time for every counter.

nieznanysprawiciel · 2024-03-19T17:05:51Z

src/counter.rs

+                    // odd step number
+                    // println!("Short request on step: {i}");
+                    let response_monitor = requests_monitor.on_request().await;
+                    // println!("Short request on step: {i}. Done.");
+                    tokio::time::sleep(delay).await;
+                    response_monitor.on_response();
+                    // println!("Short request response on step: {i}");


Please remove unused prints here and in the rest of tests

I left them because test verifies time measurement which makes stoping with debugger difficult. Printing to console every single time is unnecessary. Of course I can remove these commented prints.

nieznanysprawiciel · 2024-03-19T17:13:22Z

src/counter/combined.rs

+pub(super) struct RequestsMonitoringCounters {
+    counters: SharedCounters,
+    response_time_tx: UnboundedSender<DateTime<Utc>>,
+}


Why do you need another object for handling counters?
Can't you just implement traits for Counters struct?

Maybe I could have rename it similar to name of the module.
I wanted to have a counter implementation combining all counters which would also implement RequestMonitor trait.
At the same time I did not wanted to have top level struct Counters to implement it (and implementation of pub trait on pub struct cannot be private).

pwalski · 2024-03-20T00:59:35Z

First of all some part of counters implementations should be in yagna. Requests counters are something that every user of the library would like to use. On the other side duration counter is something more general and should probably stay in ya-runtime-ai.
Abstractions that you build should reflect this split of responsibility.

I made gsb-http-proxy to provide only a Response/RequestMonitor traits and their functions usage implementation, because I considered counters to be a part of Agreement domain and gsb-http-proxy knows nothing about it. Library knows what is a response and what is a request, and it is all it does regarding counters. The way Requests counter and Request (GPU) duration counter internally work is inseparable to its id/name, and this is a part of Agreement and I assumed it should not be inside gsb-http-proxy.
Additionally I wanted to implement counters as a tool converting golem/com/usage/vector from Agreement into corresponding currentUsage property of Activity, and hide this implementation. This conversion of a list of some names into a list of f64 values according to some opaque rules is confusing (can the list be empty? what to do on unsupported counter? can it have duplicates? (accidentally it can)).
@nieznanysprawiciel

pwalski · 2024-04-02T14:57:32Z

Work continued in PR #84

pwalski force-pushed the pwalski/request_time_counter branch 2 times, most recently from 87f6f51 to d02050d Compare March 17, 2024 20:59

pwalski added 9 commits March 18, 2024 19:54

Requests counter type

bd1ce6c

Counter module

246dde3

Rust toolchain 1.76.0

c00948c

Counters type

38db564

Duration counter

92b209b

Yagna rev update. Old counters code cleanup.

684dc08

Yagna rev update. Response counder drop handling.

abbe1c1

Tests. Request counter bugfix

a180019

Test assert detailed messages

8aeba43

pwalski force-pushed the pwalski/request_time_counter branch 2 times, most recently from b5fc2cc to 20024a6 Compare March 18, 2024 18:57

ResponseMonitors Drop handling (blocking and async env). ResponseMoni…

244b539

…tors drop test. Yagna ver update.

pwalski force-pushed the pwalski/request_time_counter branch from 20024a6 to 244b539 Compare March 18, 2024 19:19

Non blockigng ResponseMonitor. Response monitoring task. Yagna ver up…

35fd3ea

…date.

pwalski force-pushed the pwalski/request_time_counter branch from 0599d44 to 35fd3ea Compare March 19, 2024 10:00

pwalski marked this pull request as ready for review March 19, 2024 10:06

pwalski added 2 commits March 19, 2024 11:14

Cleanup

4b67bba

Unsupported counter test

2ad3bac

pwalski force-pushed the pwalski/request_time_counter branch from 0ac6add to 2ad3bac Compare March 19, 2024 10:58

pwalski commented Mar 19, 2024

View reviewed changes

pwalski changed the title ~~request time counter~~ request counters Mar 19, 2024

nieznanysprawiciel requested changes Mar 19, 2024

View reviewed changes

nieznanysprawiciel mentioned this pull request Mar 19, 2024

gsb http proxy monitor golemfactory/yagna#3108

Closed

pwalski closed this Apr 2, 2024

pwalski deleted the pwalski/request_time_counter branch April 2, 2024 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

request counters #75

request counters #75

pwalski commented Mar 15, 2024 •

edited

Loading

pwalski Mar 19, 2024

pwalski Mar 19, 2024 •

edited

Loading

nieznanysprawiciel left a comment

nieznanysprawiciel Mar 19, 2024

nieznanysprawiciel Mar 19, 2024

nieznanysprawiciel Mar 19, 2024

pwalski Mar 20, 2024

nieznanysprawiciel Mar 19, 2024

pwalski Mar 20, 2024

nieznanysprawiciel Mar 19, 2024

pwalski Mar 20, 2024 •

edited

Loading

pwalski commented Mar 20, 2024 •

edited

Loading

pwalski commented Apr 2, 2024

		let mut proxy = GsbToHttpProxy {
		// base_url: "http://10.30.13.8:7861/".to_string(),

request counters #75

request counters #75

Conversation

pwalski commented Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

pwalski Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

nieznanysprawiciel 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

pwalski Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

pwalski commented Mar 20, 2024 • edited Loading

pwalski commented Apr 2, 2024

pwalski commented Mar 15, 2024 •

edited

Loading

pwalski Mar 19, 2024 •

edited

Loading

pwalski Mar 20, 2024 •

edited

Loading

pwalski commented Mar 20, 2024 •

edited

Loading