Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: implement size-limited JSON requests for save metrics #406

Merged
merged 3 commits into from
Oct 20, 2024
Merged

feat: implement size-limited JSON requests for save metrics #406

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf13e17
feat: implement size-limited JSON requests for save metrics
pranc1ngpegasus Oct 15, 2024
4fb86fd
fix: simplify
pranc1ngpegasus Oct 17, 2024
1952571
fix: add guard clause
pranc1ngpegasus Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 53 additions & 35 deletions agent/src/services/studio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ use std::collections::VecDeque;
use serde::{Deserialize, Serialize};
use serde_json::{json, Value};

// The maximum JSON body size is actually 1MB
// We reserve 100KB as a buffer for Verifiable Credential capacity
const JSON_BODY_MAX_SIZE: usize = 900_000;

#[derive(Deserialize)]
pub struct EmptyResponse {}

Expand Down Expand Up @@ -423,46 +427,60 @@ struct MetricsWithTimestampStr {

impl MetricStoreRepository for Studio {
async fn save(&self, request: VecDeque<MetricsWithTimestamp>) -> anyhow::Result<()> {
let metrics_str = request
.into_iter()
.map(|m| MetricsWithTimestampStr {
timestamp: m.timestamp,
metrics: m
.metrics
.into_iter()
.map(|metric| MetricStr {
metric_type: metric.metric_type.to_string(),
value: metric.value,
})
.collect::<Vec<MetricStr>>(),
})
.collect::<Vec<MetricsWithTimestampStr>>();

let my_did = self.did_accessor.get_my_did();
let my_keyring = self.did_accessor.get_my_keyring();
let mut metrics = request;
while !metrics.is_empty() {
let my_did = self.did_accessor.get_my_did();
let my_keyring = self.did_accessor.get_my_keyring();
let mut metrics_str = Vec::new();
let mut current_size = 0;

while let Some(m) = metrics.pop_front() {
let metrics_with_timestamp_str = MetricsWithTimestampStr {
timestamp: m.timestamp,
metrics: m
.metrics
.iter()
.map(|metric| MetricStr {
metric_type: metric.metric_type.to_string(),
value: metric.value,
})
.collect::<Vec<MetricStr>>(),
};

let item_size = serde_json::to_string(&metrics_with_timestamp_str)?.len();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to assert that item_size does not exceed JSON_BODY_MAX_SIZE like this.

                if item_size > JSON_BODY_MAX_SIZE {
                    anyhow::bail!("Invalid item size");
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback.
item_size is size of JSON encoded each metrics.
current_size is total size of JSON encoded request body.
So I was implemented comparison between current_size and JSON_BODY_MAX_SIZE.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that your implementation has the implicit assumption: item_size does not exceed JSON_BODY_MAX_SIZE.
So I thought we should explicitly state that like below.

                if item_size > JSON_BODY_MAX_SIZE {
                    anyhow::bail!("Invalid item size");
                }
                if current_size + item_size > JSON_BODY_MAX_SIZE {
                    metrics.push_front(m);
                    break;
                }

Note: If any element has item_size > JSON_BODY_MAX_SIZE, it will loop infinitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback.
I have fixed the program in the following commit:
1952571

if current_size + item_size > JSON_BODY_MAX_SIZE {
metrics.push_front(m);
break;
}
current_size += item_size;
metrics_str.push(metrics_with_timestamp_str);
}

let model = VerifiableCredentials::new(my_did, json!(metrics_str), chrono::Utc::now());
let payload = DidVcService::generate(&self.did_repository, model, &my_keyring)
.context("failed to generate payload")?;
let model = VerifiableCredentials::new(my_did, json!(metrics_str), chrono::Utc::now());
let payload = DidVcService::generate(&self.did_repository, model, &my_keyring)
.context("failed to generate payload")?;

let payload = serde_json::to_string(&payload).context("failed to serialize")?;
let res = self.http_client.post("/v1/metrics", &payload).await?;
let payload = serde_json::to_string(&payload).context("failed to serialize")?;
let res = self.http_client.post("/v1/metrics", &payload).await?;

let status = res.status();
let json: Value = res.json().await.context("Failed to read response body")?;
let message = if let Some(message) = json.get("message").map(|v| v.to_string()) {
message
} else {
"".to_string()
};
match status {
reqwest::StatusCode::OK => Ok(()),
reqwest::StatusCode::NOT_FOUND => anyhow::bail!("StatusCode=404, {}", message),
reqwest::StatusCode::INTERNAL_SERVER_ERROR => {
anyhow::bail!("StatusCode=500, {}", message);
let status = res.status();
let json: Value = res.json().await.context("Failed to read response body")?;
let message = if let Some(message) = json.get("message").map(|v| v.to_string()) {
message
} else {
"".to_string()
};
match status {
reqwest::StatusCode::OK => continue,
reqwest::StatusCode::NOT_FOUND => anyhow::bail!("StatusCode=404, {}", message),
reqwest::StatusCode::INTERNAL_SERVER_ERROR => {
anyhow::bail!("StatusCode=500, {}", message);
}
other => anyhow::bail!("StatusCode={other}, {}", message),
}
other => anyhow::bail!("StatusCode={other}, {}", message),
}

Ok(())
}
}

Expand Down
Loading