Feature: update client statistics#68
Conversation
# Conflicts: # forward-proxy/src/handler/mod.rs
stravid87
left a comment
stravid87
left a comment
There was a problem hiding this comment.
Great work. I'm happy with this once again.
Also, I'm happy(?) to report that this is the first time I feel like I can offer legitimate criticism to stretch your skills? I'm still insecure about my ability to give feedback so I invite you to push back but: in forward-proxy/src/statistics/mod.rs, can you remove the .await calls? Ideally, stats logging will happen entirely in parallel. As a fallback, if we error out, allow the request to go through and we (the Layer8 team) will simply forgo that monetization opportunity.
| - DOCKER_INFLUXDB_INIT_MODE=setup | ||
| - DOCKER_INFLUXDB_INIT_USERNAME=admin | ||
| - DOCKER_INFLUXDB_INIT_PASSWORD=12341234 | ||
| - DOCKER_INFLUXDB_INIT_ORG=globeandcitizen |
There was a problem hiding this comment.
Seeing things like this, "ORG=globeandcitizen", always makes me smile.
| @@ -11,7 +11,9 @@ pub struct FPConfig { | |||
| #[serde(flatten)] | |||
| pub tls_config: TlsConfig, | |||
| #[serde(flatten)] // This flattens the HandlerConfig fields into this struct | |||
|
|
||
| #[derive(Clone, Debug, Default)] | ||
| pub struct IntFPSession { | ||
| pub client_id: String, |
There was a problem hiding this comment.
Got it. You add here the client_id used to track InfluxDB
| @@ -101,11 +102,12 @@ impl ForwardHandler { | |||
| #[derive(Deserialize, Debug)] | |||
| struct AuthServerResponse { | |||
There was a problem hiding this comment.
In layer8, the corresponding struct was simply called, x509_certificate. Here it's called AuthServerResponse and that's a better name.
| } | ||
|
|
||
| let cert: AuthServerResponse = res.json().await.map_err(|err| { | ||
| let auth_res: AuthServerResponse = res.json().await.map_err(|err| { |
There was a problem hiding this comment.
Yup -- better naming here too.
| ctx: &Layer8Context, | ||
| response_status: u16, | ||
| ) -> Result<(), Box<dyn Error + Sync + Send>> { | ||
| self.increase_total_request(client_id).await?; |
There was a problem hiding this comment.
Here true async code would be better. Blocking with await could probably be avoided with a clever solution that, if an error occurs, we handle it gracefully. The 'cost' might be that we miss out on the monetization of an incremented requests but the benefit might be more performant code.
| "/proxy" => { | ||
| self.add_total_byte_transferred( | ||
| client_id, | ||
| (ctx.get_request_body().len() + ctx.get_response_body().len()) as i64, |
| self.increase_total_success(client_id).await | ||
| } | ||
| "/init-tunnel" => self.increase_total_tunnel_initiated(client_id).await, | ||
| _ => Ok(()), |
| ) | ||
| .await?; | ||
|
|
||
| self.increase_total_success(client_id).await |
There was a problem hiding this comment.
Can we remove awaits when tracking stats?
dtpthao
force-pushed
the
thao/statistics
branch
from
40f64cf to
623c4e1
Compare
dtpthao
force-pushed
the
thao/statistics
branch
from
58185af to
2e5628a
Compare
3 participants
Result: