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.

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

feat: upgrade logging level of serialization errors to actual errors #723

Merged
merged 2 commits into from
Feb 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion server/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ async fn build_edge(
refresher_mode,
client_meta_information,
args.delta,
args.delta_diff
args.delta_diff,
);
let feature_refresher = Arc::new(FeatureRefresher::new(
unleash_client,
Expand Down
4 changes: 2 additions & 2 deletions server/src/feature_cache.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use crate::types::EdgeToken;
use dashmap::DashMap;
use tokio::sync::broadcast;
use unleash_types::client_features::ClientFeaturesDelta;
use unleash_types::{
client_features::{ClientFeature, ClientFeatures, Segment},
Deduplicate,
};
use unleash_types::client_features::ClientFeaturesDelta;
use crate::types::EdgeToken;

#[derive(Debug, Clone)]
pub enum UpdateType {
Expand Down
2 changes: 1 addition & 1 deletion server/src/http/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
pub mod background_send_metrics;
pub mod broadcaster;
pub(crate) mod headers;
pub mod unleash_client;
pub mod refresher;
pub mod unleash_client;
36 changes: 19 additions & 17 deletions server/src/http/refresher/delta_refresher.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
use std::time::Duration;
use eventsource_client::Client;
use actix_web::http::header::EntityTag;
use reqwest::StatusCode;
use eventsource_client::Client;
use futures::TryStreamExt;
use reqwest::StatusCode;
use std::time::Duration;
use tracing::{debug, info, warn};
use unleash_types::client_features::{ClientFeaturesDelta};
use unleash_types::client_features::ClientFeaturesDelta;
use unleash_yggdrasil::EngineState;

use crate::error::{EdgeError, FeatureError};
use crate::http::headers::{UNLEASH_APPNAME_HEADER, UNLEASH_CLIENT_SPEC_HEADER, UNLEASH_INSTANCE_ID_HEADER};
use crate::types::{ClientFeaturesDeltaResponse, ClientFeaturesRequest, EdgeToken, TokenRefresh};
use crate::http::headers::{
UNLEASH_APPNAME_HEADER, UNLEASH_CLIENT_SPEC_HEADER, UNLEASH_INSTANCE_ID_HEADER,
};
use crate::http::refresher::feature_refresher::FeatureRefresher;
use crate::http::unleash_client::ClientMetaInformation;
use crate::tokens::cache_key;
use crate::types::{ClientFeaturesDeltaResponse, ClientFeaturesRequest, EdgeToken, TokenRefresh};

impl FeatureRefresher {
async fn handle_client_features_delta_updated(
Expand Down Expand Up @@ -230,6 +232,10 @@ impl FeatureRefresher {

#[cfg(test)]
mod tests {
use crate::feature_cache::FeatureCache;
use crate::http::refresher::feature_refresher::FeatureRefresher;
use crate::http::unleash_client::{ClientMetaInformation, UnleashClient};
use crate::types::EdgeToken;
use actix_http::header::IF_NONE_MATCH;
use actix_http::HttpService;
use actix_http_test::{test_server, TestServer};
Expand All @@ -240,11 +246,10 @@ mod tests {
use chrono::Duration;
use dashmap::DashMap;
use std::sync::Arc;
use crate::feature_cache::FeatureCache;
use crate::http::refresher::feature_refresher::FeatureRefresher;
use crate::http::unleash_client::{ClientMetaInformation, UnleashClient};
use crate::types::EdgeToken;
use unleash_types::client_features::{ClientFeature, ClientFeatures, ClientFeaturesDelta, Constraint, DeltaEvent, Operator, Segment};
use unleash_types::client_features::{
ClientFeature, ClientFeatures, ClientFeaturesDelta, Constraint, DeltaEvent, Operator,
Segment,
};
use unleash_yggdrasil::EngineState;

#[actix_web::test]
Expand All @@ -265,7 +270,7 @@ mod tests {
strict: false,
streaming: false,
delta: true,
delta_diff : false,
delta_diff: false,
client_meta_information: ClientMetaInformation::test_config(),
});
let mut delta_features = ClientFeatures::create_from_delta(&revision(1));
Expand Down Expand Up @@ -359,7 +364,6 @@ mod tests {
}
}


async fn return_client_features_delta(etag_header: Option<String>) -> HttpResponse {
match etag_header {
Some(value) => match value.as_str() {
Expand Down Expand Up @@ -389,10 +393,8 @@ mod tests {
))),
|_| AppConfig::default(),
))
.tcp()
.tcp()
})
.await
.await
}


}
44 changes: 24 additions & 20 deletions server/src/http/refresher/feature_refresher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ use crate::filters::{filter_client_features, FeatureFilterSet};
use crate::http::headers::{
UNLEASH_APPNAME_HEADER, UNLEASH_CLIENT_SPEC_HEADER, UNLEASH_INSTANCE_ID_HEADER,
};
use crate::types::{build, ClientFeaturesDeltaResponse, EdgeResult, TokenType, TokenValidationStatus};
use crate::types::{
build, ClientFeaturesDeltaResponse, EdgeResult, TokenType, TokenValidationStatus,
};
use crate::{
persistence::EdgePersistence,
tokens::{cache_key, simplify},
Expand Down Expand Up @@ -122,7 +124,7 @@ impl FeatureRefreshConfig {
mode,
client_meta_information,
delta,
delta_diff
delta_diff,
}
}
}
Expand Down Expand Up @@ -347,7 +349,7 @@ impl FeatureRefresher {

match serde_json::from_str(&event.data) {
Ok(features) => { refresher.handle_client_features_updated(&token, features, None).await; }
Err(e) => { warn!("Could not parse features response to internal representation: {e:?}");
Err(e) => { tracing::error!("Could not parse features response to internal representation: {e:?}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this is the relevant change, shouldn't we use the same match serde_json::from_str in line 424 and 425 when reading deltas?

}
}
}
Expand Down Expand Up @@ -411,16 +413,13 @@ impl FeatureRefresher {
if let Some(client_features) = self.features_cache.get(&key).as_ref() {
if let Ok(ClientFeaturesDeltaResponse::Updated(delta_features, _etag)) = delta_result {
let c_features = &client_features.features;
let d_features = delta_features
.events
.iter()
.find_map(|event| {
if let DeltaEvent::Hydration { features, .. } = event {
Some(features)
} else {
None
}
});
let d_features = delta_features.events.iter().find_map(|event| {
if let DeltaEvent::Hydration { features, .. } = event {
Some(features)
} else {
None
}
});

let delta_json = serde_json::to_value(d_features).unwrap();
let client_json = serde_json::to_value(c_features).unwrap();
Expand Down Expand Up @@ -475,7 +474,6 @@ impl FeatureRefresher {
} else {
self.refresh_single(refresh).await;
}

}
}

Expand Down Expand Up @@ -1043,7 +1041,8 @@ mod tests {
let example_features = features_from_disk("../examples/features.json");
let cache_key = cache_key(&token);
let mut engine_state = EngineState::default();
let warnings = engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
let warnings =
engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
upstream_features_cache.insert(cache_key.clone(), example_features.clone());
upstream_engine_cache.insert(cache_key.clone(), engine_state);
let mut server = client_api_test_server(
Expand Down Expand Up @@ -1099,7 +1098,8 @@ mod tests {
let example_features = features_from_disk("../examples/features.json");
let cache_key = cache_key(&valid_token);
let mut engine_state = EngineState::default();
let warnings = engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
let warnings =
engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
upstream_features_cache.insert(cache_key.clone(), example_features.clone());
upstream_engine_cache.insert(cache_key.clone(), engine_state);
let server = client_api_test_server(
Expand Down Expand Up @@ -1144,7 +1144,8 @@ mod tests {
let example_features = features_from_disk("../examples/hostedexample.json");
let cache_key = cache_key(&dx_token);
let mut engine_state = EngineState::default();
let warnings = engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
let warnings =
engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
upstream_features_cache.insert(cache_key.clone(), example_features.clone());
upstream_engine_cache.insert(cache_key.clone(), engine_state);
let server = client_api_test_server(
Expand Down Expand Up @@ -1194,7 +1195,8 @@ mod tests {
let cache_key = cache_key(&dx_token);
upstream_features_cache.insert(cache_key.clone(), example_features.clone());
let mut engine_state = EngineState::default();
let warnings = engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
let warnings =
engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
upstream_engine_cache.insert(cache_key, engine_state);
let server = client_api_test_server(
upstream_token_cache,
Expand Down Expand Up @@ -1256,7 +1258,8 @@ mod tests {
let cache_key = cache_key(&dx_token);
upstream_features_cache.insert(cache_key.clone(), example_features.clone());
let mut engine_state = EngineState::default();
let warnings = engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
let warnings =
engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
upstream_engine_cache.insert(cache_key, engine_state);
let server = client_api_test_server(
upstream_token_cache,
Expand Down Expand Up @@ -1364,7 +1367,8 @@ mod tests {
let cache_key = cache_key(&eg_token);
upstream_features_cache.insert(cache_key.clone(), example_features.clone());
let mut engine_state = EngineState::default();
let warnings = engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
let warnings =
engine_state.take_state(UpdateMessage::FullResponse(example_features.clone()));
upstream_engine_cache.insert(cache_key.clone(), engine_state);
let server = client_api_test_server(
upstream_token_cache,
Expand Down
2 changes: 1 addition & 1 deletion server/src/http/refresher/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
pub mod feature_refresher;
pub mod delta_refresher;
pub mod feature_refresher;
25 changes: 12 additions & 13 deletions server/src/http/unleash_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,6 @@ impl UnleashClient {

#[cfg(test)]
pub fn new_insecure(server_url: &str) -> Result<Self, EdgeError> {

Ok(Self {
urls: UnleashUrls::from_str(server_url)?,
backing_client: new_reqwest_client(
Expand Down Expand Up @@ -609,17 +608,6 @@ mod tests {
use std::path::PathBuf;
use std::str::FromStr;

use actix_http::{body::MessageBody, HttpService, TlsAcceptorConfig};
use actix_http_test::{test_server, TestServer};
use actix_middleware_etag::Etag;
use actix_service::map_config;
use actix_web::{
dev::{AppConfig, ServiceRequest, ServiceResponse},
http::header::EntityTag,
web, App, HttpResponse,
};
use chrono::Duration;
use unleash_types::client_features::{ClientFeature, ClientFeatures};
use crate::cli::ClientIdentity;
use crate::http::unleash_client::new_reqwest_client;
use crate::{
Expand All @@ -631,8 +619,19 @@ mod tests {
ValidateTokensRequest,
},
};
use actix_http::{body::MessageBody, HttpService, TlsAcceptorConfig};
use actix_http_test::{test_server, TestServer};
use actix_middleware_etag::Etag;
use actix_service::map_config;
use actix_web::{
dev::{AppConfig, ServiceRequest, ServiceResponse},
http::header::EntityTag,
web, App, HttpResponse,
};
use chrono::Duration;
use unleash_types::client_features::{ClientFeature, ClientFeatures};

use super::{EdgeTokens, UnleashClient, ClientMetaInformation};
use super::{ClientMetaInformation, EdgeTokens, UnleashClient};

impl ClientFeaturesRequest {
pub(crate) fn new(api_key: String, etag: Option<String>) -> Self {
Expand Down
4 changes: 1 addition & 3 deletions server/src/metrics/client_metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,7 @@ pub struct MetricsCache {
}

pub(crate) fn size_of_batch(batch: &MetricsBatch) -> usize {
serde_json::to_string(batch)
.map(|s| s.len())
.unwrap_or(0)
serde_json::to_string(batch).map(|s| s.len()).unwrap_or(0)
}

pub(crate) fn register_client_application(
Expand Down
2 changes: 1 addition & 1 deletion server/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use chrono::{DateTime, Duration, Utc};
use dashmap::DashMap;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use shadow_rs::shadow;
use unleash_types::client_features::{ClientFeatures, ClientFeaturesDelta};
use unleash_types::client_features::Context;
use unleash_types::client_features::{ClientFeatures, ClientFeaturesDelta};
use unleash_types::client_metrics::{ClientApplication, ClientMetricsEnv};
use unleash_yggdrasil::EngineState;
use utoipa::{IntoParams, ToSchema};
Expand Down
5 changes: 4 additions & 1 deletion server/src/urls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ mod tests {
assert_eq!(urls.api_url.to_string(), api_url);
assert_eq!(urls.client_api_url.to_string(), client_url);
assert_eq!(urls.client_features_url.to_string(), client_features_url);
assert_eq!(urls.client_features_delta_url.to_string(), client_features_delta_url);
assert_eq!(
urls.client_features_delta_url.to_string(),
client_features_delta_url
);
}
}
2 changes: 1 addition & 1 deletion server/tests/streaming_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ mod streaming_test {
strict: true,
dynamic: false,
delta: false,
delta_diff:false,
delta_diff: false,
prometheus_remote_write_url: None,
prometheus_push_interval: 60,
prometheus_username: None,
Expand Down