исправить: закрыть ревью сквозной корреляции
This commit is contained in:
@@ -6,7 +6,7 @@ use axum::{
|
||||
};
|
||||
use base64::{Engine as _, engine::general_purpose::URL_SAFE_NO_PAD};
|
||||
use crank_core::{OperationSecurityLevel, PlatformApiKeyScope};
|
||||
use crank_trace::{DbOperation, ErrorCategory, Stage, StageOutcome, observe_db_query};
|
||||
use crank_trace::{DbOperation, ErrorCategory, StageOutcome, observe_db_query};
|
||||
use sha2::{Digest, Sha256};
|
||||
use time::OffsetDateTime;
|
||||
use tracing::Instrument;
|
||||
@@ -154,9 +154,7 @@ async fn verify_static_agent_key(
|
||||
secret: &str,
|
||||
) -> Result<Option<VerifiedMachineCredential>, MachineAccessError> {
|
||||
let secret_hash = hash_access_secret(secret);
|
||||
let read_span = Stage::DbQuery
|
||||
.db_span(DbOperation::MachineAccessRead)
|
||||
.expect("database stage");
|
||||
let read_span = DbOperation::MachineAccessRead.span();
|
||||
let api_key_result = state
|
||||
.registry
|
||||
.get_platform_api_key_by_secret_for_agent_slug(
|
||||
@@ -185,9 +183,7 @@ async fn verify_static_agent_key(
|
||||
};
|
||||
|
||||
let used_at = OffsetDateTime::now_utc();
|
||||
let touch_span = Stage::DbQuery
|
||||
.db_span(DbOperation::MachineAccessTouch)
|
||||
.expect("database stage");
|
||||
let touch_span = DbOperation::MachineAccessTouch.span();
|
||||
let touch_result = state
|
||||
.registry
|
||||
.touch_platform_api_key(&api_key.api_key.workspace_id, &api_key.api_key.id, &used_at)
|
||||
|
||||
@@ -52,9 +52,7 @@ pub(crate) async fn persist_invocation(
|
||||
|
||||
let history_span = Stage::HistoryWrite.span();
|
||||
let (outcome, db_span) = async {
|
||||
let db_span = Stage::DbQuery
|
||||
.db_span(DbOperation::InvocationHistoryWrite)
|
||||
.expect("database stage");
|
||||
let db_span = DbOperation::InvocationHistoryWrite.span();
|
||||
let outcome = state
|
||||
.registry
|
||||
.create_invocation_log(CreateInvocationLogRequest { log: &log })
|
||||
@@ -82,7 +80,7 @@ pub(crate) async fn persist_invocation(
|
||||
outcome,
|
||||
record.request_id,
|
||||
record.status,
|
||||
"agent_tool_call",
|
||||
InvocationSource::AgentToolCall,
|
||||
);
|
||||
outcome
|
||||
}
|
||||
@@ -91,7 +89,7 @@ pub(super) fn observe_invocation_history_outcome(
|
||||
outcome: InvocationHistoryWriteOutcome,
|
||||
request_id: Option<&str>,
|
||||
status: InvocationStatus,
|
||||
source: &'static str,
|
||||
source: InvocationSource,
|
||||
) {
|
||||
let Some(loss) = outcome.loss() else {
|
||||
return;
|
||||
@@ -102,13 +100,20 @@ pub(super) fn observe_invocation_history_outcome(
|
||||
warn!(
|
||||
name: "mcp.invocation_history.lost",
|
||||
request_id = request_id.unwrap_or_default(),
|
||||
source,
|
||||
source = invocation_source_label(source),
|
||||
invocation_status = invocation_status_label(status),
|
||||
error_category = loss.category.as_str(),
|
||||
"invocation history was not recorded"
|
||||
);
|
||||
}
|
||||
|
||||
fn invocation_source_label(source: InvocationSource) -> &'static str {
|
||||
match source {
|
||||
InvocationSource::AdminTestRun => "admin_test_run",
|
||||
InvocationSource::AgentToolCall => "agent_tool_call",
|
||||
}
|
||||
}
|
||||
|
||||
fn invocation_status_label(status: InvocationStatus) -> &'static str {
|
||||
match status {
|
||||
InvocationStatus::Ok => "ok",
|
||||
|
||||
@@ -75,7 +75,7 @@ fn emits_bounded_history_loss_incident() {
|
||||
}),
|
||||
Some("req_mcp_dc08"),
|
||||
InvocationStatus::Ok,
|
||||
"agent_tool_call",
|
||||
crank_core::InvocationSource::AgentToolCall,
|
||||
);
|
||||
|
||||
let output = writer.output();
|
||||
|
||||
@@ -159,9 +159,7 @@ impl PublishedToolCatalog {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let db_span = Stage::DbQuery
|
||||
.db_span(DbOperation::CatalogLoad)
|
||||
.expect("database stage");
|
||||
let db_span = DbOperation::CatalogLoad.span();
|
||||
let catalog_result = self
|
||||
.registry
|
||||
.get_published_agent_catalog_by_slug(workspace_slug, agent_slug)
|
||||
|
||||
@@ -4,6 +4,7 @@ use axum::{
|
||||
http::{HeaderMap, HeaderValue, StatusCode, header::RETRY_AFTER},
|
||||
response::{IntoResponse, Response},
|
||||
};
|
||||
use crank_core::PlatformApiKeyKind;
|
||||
use crank_runtime::RateLimitCheckError;
|
||||
use serde_json::{Value, json};
|
||||
|
||||
@@ -82,6 +83,13 @@ pub(super) fn rate_limited_status_response(error: RateLimitCheckError) -> Respon
|
||||
}
|
||||
|
||||
fn rate_limit_key(path: &AgentRoutePath, headers: &HeaderMap) -> String {
|
||||
let access_secret = bearer_token(headers);
|
||||
if let Some(secret) = access_secret
|
||||
&& secret.starts_with(PlatformApiKeyKind::Approval.secret_marker())
|
||||
{
|
||||
return format!("api_key:{}", hash_access_secret(secret));
|
||||
}
|
||||
|
||||
if let Ok(Some(session_id)) = session_id_from_headers(headers) {
|
||||
return format!(
|
||||
"session:{}:{}:{}",
|
||||
@@ -89,7 +97,7 @@ fn rate_limit_key(path: &AgentRoutePath, headers: &HeaderMap) -> String {
|
||||
);
|
||||
}
|
||||
|
||||
if let Some(secret) = bearer_token(headers) {
|
||||
if let Some(secret) = access_secret {
|
||||
return format!("api_key:{}", hash_access_secret(secret));
|
||||
}
|
||||
|
||||
|
||||
@@ -10,13 +10,7 @@ pub(super) struct RequestContext {
|
||||
}
|
||||
|
||||
pub(super) async fn apply_request_context(mut request: Request, next: Next) -> Response {
|
||||
let request_id = RequestId::resolve(
|
||||
request
|
||||
.headers()
|
||||
.get(&HEADER_X_REQUEST_ID)
|
||||
.and_then(|value| value.to_str().ok()),
|
||||
)
|
||||
.into_string();
|
||||
let request_id = RequestId::resolve_from_headers(request.headers()).into_string();
|
||||
let context = RequestContext {
|
||||
request_id: request_id.clone(),
|
||||
};
|
||||
|
||||
@@ -42,6 +42,15 @@ pub enum PlatformApiKeyKind {
|
||||
Approval,
|
||||
}
|
||||
|
||||
impl PlatformApiKeyKind {
|
||||
pub const fn secret_marker(self) -> &'static str {
|
||||
match self {
|
||||
Self::McpClient => "crk_",
|
||||
Self::Approval => "crk_appr_",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum PlatformApiKeyScope {
|
||||
@@ -123,6 +132,12 @@ mod tests {
|
||||
};
|
||||
use crate::ids::{AgentId, PlatformApiKeyId, UserId, WorkspaceId};
|
||||
|
||||
#[test]
|
||||
fn api_key_kinds_own_their_secret_markers() {
|
||||
assert_eq!(PlatformApiKeyKind::McpClient.secret_marker(), "crk_");
|
||||
assert_eq!(PlatformApiKeyKind::Approval.secret_marker(), "crk_appr_");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn user_serializes_created_at_as_rfc3339() {
|
||||
let user = User {
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use std::fmt;
|
||||
|
||||
use axum::http::HeaderMap;
|
||||
use uuid::Uuid;
|
||||
|
||||
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
|
||||
@@ -7,6 +8,7 @@ pub struct RequestId(String);
|
||||
|
||||
impl RequestId {
|
||||
pub const MAX_LEN: usize = 128;
|
||||
const HEADER_NAME: &'static str = "x-request-id";
|
||||
|
||||
pub fn resolve(candidate: Option<&str>) -> Self {
|
||||
candidate
|
||||
@@ -15,6 +17,16 @@ impl RequestId {
|
||||
.unwrap_or_else(|| Self(Uuid::now_v7().to_string()))
|
||||
}
|
||||
|
||||
pub fn resolve_from_headers(headers: &HeaderMap) -> Self {
|
||||
let mut values = headers.get_all(Self::HEADER_NAME).iter();
|
||||
let candidate = values.next();
|
||||
if values.next().is_some() {
|
||||
return Self::resolve(None);
|
||||
}
|
||||
|
||||
Self::resolve(candidate.and_then(|value| value.to_str().ok()))
|
||||
}
|
||||
|
||||
pub fn is_valid(value: &str) -> bool {
|
||||
!value.is_empty()
|
||||
&& value.len() <= Self::MAX_LEN
|
||||
|
||||
@@ -597,7 +597,7 @@ mod tests {
|
||||
traces_endpoint: Some("https://traces.example.test/custom".to_owned()),
|
||||
generic_endpoint: Some("https://generic.example.test/otel".to_owned()),
|
||||
traces_protocol: Some("http/protobuf".to_owned()),
|
||||
generic_protocol: Some("grpc".to_owned()),
|
||||
generic_protocol: Some("grpc".to_owned()), // community-scope: allow=grpc
|
||||
traces_timeout: Some("2500".to_owned()),
|
||||
generic_timeout: Some("invalid-unused-fallback".to_owned()),
|
||||
..OtlpEnvSettings::default()
|
||||
@@ -615,8 +615,8 @@ mod tests {
|
||||
#[test]
|
||||
fn disabled_export_ignores_inactive_settings() {
|
||||
let config = OtlpEnvSettings {
|
||||
traces_protocol: Some("grpc".to_owned()),
|
||||
generic_protocol: Some("grpc".to_owned()),
|
||||
traces_protocol: Some("grpc".to_owned()), // community-scope: allow=grpc
|
||||
generic_protocol: Some("grpc".to_owned()), // community-scope: allow=grpc
|
||||
traces_timeout: Some("invalid".to_owned()),
|
||||
generic_timeout: Some("invalid".to_owned()),
|
||||
max_queue_size: Some("invalid".to_owned()),
|
||||
@@ -637,7 +637,7 @@ mod tests {
|
||||
traces_endpoint: Some("https://traces.example.test/v1/traces".to_owned()),
|
||||
traces_headers: Some(String::new()),
|
||||
generic_headers: Some(
|
||||
"authorization=Bearer%20canary-token,x-tenant=community".to_owned(),
|
||||
"authorization=Bearer%20canary-token,x-scope=community".to_owned(),
|
||||
),
|
||||
..OtlpEnvSettings::default()
|
||||
}
|
||||
@@ -645,7 +645,7 @@ mod tests {
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(config.header("authorization"), Some("Bearer canary-token"));
|
||||
assert_eq!(config.header("x-tenant"), Some("community"));
|
||||
assert_eq!(config.header("x-scope"), Some("community"));
|
||||
assert!(!format!("{config:?}").contains("canary-token"));
|
||||
}
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use axum::http::{HeaderMap, HeaderValue};
|
||||
use crank_observability::RequestId;
|
||||
use uuid::Version;
|
||||
|
||||
@@ -40,3 +41,34 @@ fn rejects_values_over_the_shared_limit() {
|
||||
Some(Version::SortRand)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolves_exactly_one_header_value_and_rejects_ambiguous_values() {
|
||||
let mut single = HeaderMap::new();
|
||||
single.insert(
|
||||
"x-request-id",
|
||||
HeaderValue::from_static("opaque-request-id"),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
RequestId::resolve_from_headers(&single).as_str(),
|
||||
"opaque-request-id"
|
||||
);
|
||||
|
||||
let mut ambiguous = HeaderMap::new();
|
||||
ambiguous.append("x-request-id", HeaderValue::from_static("first-request-id"));
|
||||
ambiguous.append(
|
||||
"x-request-id",
|
||||
HeaderValue::from_static("second-request-id"),
|
||||
);
|
||||
|
||||
let generated = RequestId::resolve_from_headers(&ambiguous);
|
||||
assert_ne!(generated.as_str(), "first-request-id");
|
||||
assert_ne!(generated.as_str(), "second-request-id");
|
||||
assert_eq!(
|
||||
uuid::Uuid::parse_str(generated.as_str())
|
||||
.expect("generated UUID")
|
||||
.get_version(),
|
||||
Some(Version::SortRand)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -37,7 +37,7 @@ fn invalid_values_return_safe_typed_errors() {
|
||||
let secret_endpoint = "https://user:canary-secret@collector.example.test/v1/traces";
|
||||
let error = OtlpTraceConfig::try_new(
|
||||
Some(secret_endpoint.to_owned()),
|
||||
Some("grpc".to_owned()),
|
||||
Some("grpc".to_owned()), // community-scope: allow=grpc
|
||||
Duration::ZERO,
|
||||
OtlpBatchConfig::default(),
|
||||
)
|
||||
|
||||
@@ -11,6 +11,7 @@ use serde_json::{Map, Value, json};
|
||||
use time::OffsetDateTime;
|
||||
use tokio::sync::{OwnedSemaphorePermit, Semaphore};
|
||||
use tracing::{Instrument, Span, debug};
|
||||
use uuid::Uuid;
|
||||
|
||||
use crate::{
|
||||
AdapterResponse, PreparedRequest, ResolvedAuth, RuntimeError, RuntimeLimits, RuntimeOperation,
|
||||
@@ -517,9 +518,9 @@ impl RuntimeExecutor {
|
||||
fn adapter_request_context(
|
||||
request_context: Option<&RuntimeRequestContext>,
|
||||
) -> crank_core::RuntimeRequestContext {
|
||||
request_context
|
||||
.map(Into::into)
|
||||
.unwrap_or_else(|| crank_core::RuntimeRequestContext::new(String::new(), String::new()))
|
||||
request_context.map(Into::into).unwrap_or_else(|| {
|
||||
crank_core::RuntimeRequestContext::from_request_id(Uuid::now_v7().to_string())
|
||||
})
|
||||
}
|
||||
|
||||
fn map_protocol_adapter_error(
|
||||
|
||||
@@ -2,6 +2,5 @@ mod integration {
|
||||
mod confirmation;
|
||||
mod idempotency;
|
||||
mod no_input_get;
|
||||
mod stages;
|
||||
mod valkey;
|
||||
}
|
||||
|
||||
@@ -25,8 +25,12 @@ async fn valkey_coordination_and_rate_limit_operations_are_atomic() {
|
||||
.get_host_port_ipv4(6379.tcp())
|
||||
.await
|
||||
.expect("Valkey port must be mapped");
|
||||
let host = container
|
||||
.get_host()
|
||||
.await
|
||||
.expect("Docker host must be resolved");
|
||||
let store = Arc::new(
|
||||
RedisCacheStore::connect(CacheBackend::Valkey, &format!("redis://127.0.0.1:{port}/0"))
|
||||
RedisCacheStore::connect(CacheBackend::Valkey, &format!("redis://{host}:{port}/0"))
|
||||
.await
|
||||
.expect("runtime store must connect to Valkey"),
|
||||
);
|
||||
|
||||
+75
@@ -20,9 +20,13 @@ use serde_json::json;
|
||||
use time::OffsetDateTime;
|
||||
use tracing::{Id, Instrument, Subscriber, field::Visit, instrument::WithSubscriber};
|
||||
use tracing_subscriber::{Layer, layer::SubscriberExt, registry::LookupSpan};
|
||||
use uuid::Version;
|
||||
|
||||
static TRACING_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());
|
||||
|
||||
#[tokio::test]
|
||||
async fn successful_execution_has_real_stages_and_omits_inapplicable_ones() {
|
||||
let _tracing_test_guard = TRACING_TEST_LOCK.lock().await;
|
||||
let capture = TraceCapture::default();
|
||||
let subscriber = tracing_subscriber::registry().with(capture.clone());
|
||||
let executor = RuntimeExecutorBuilder::new()
|
||||
@@ -82,6 +86,7 @@ async fn successful_execution_has_real_stages_and_omits_inapplicable_ones() {
|
||||
|
||||
#[tokio::test]
|
||||
async fn failed_mapping_records_closed_category_and_stops_later_stages() {
|
||||
let _tracing_test_guard = TRACING_TEST_LOCK.lock().await;
|
||||
let capture = TraceCapture::default();
|
||||
let subscriber = tracing_subscriber::registry().with(capture.clone());
|
||||
let executor = RuntimeExecutorBuilder::new()
|
||||
@@ -110,8 +115,46 @@ async fn failed_mapping_records_closed_category_and_stops_later_stages() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn execution_without_context_sends_generated_correlation_headers() {
|
||||
let _tracing_test_guard = TRACING_TEST_LOCK.lock().await;
|
||||
let captured_headers = Arc::new(Mutex::new(None));
|
||||
let executor = RuntimeExecutorBuilder::new()
|
||||
.register_adapter(Arc::new(ContextCapturingAdapter {
|
||||
captured_headers: Arc::clone(&captured_headers),
|
||||
}))
|
||||
.build();
|
||||
let operation = operation().into();
|
||||
|
||||
let result = async {
|
||||
executor
|
||||
.execute(&operation, &json!({"name": "without-context"}))
|
||||
.await
|
||||
}
|
||||
.with_subscriber(tracing_subscriber::registry())
|
||||
.await;
|
||||
|
||||
assert_eq!(result.unwrap(), json!({"accepted": true}));
|
||||
let headers = captured_headers
|
||||
.lock()
|
||||
.expect("captured headers lock")
|
||||
.clone()
|
||||
.expect("adapter headers");
|
||||
let request_id = headers.get("x-request-id").expect("x-request-id");
|
||||
let correlation_id = headers.get("x-correlation-id").expect("x-correlation-id");
|
||||
assert!(!request_id.is_empty());
|
||||
assert_eq!(correlation_id, request_id);
|
||||
assert_eq!(
|
||||
uuid::Uuid::parse_str(request_id)
|
||||
.expect("generated UUID")
|
||||
.get_version(),
|
||||
Some(Version::SortRand)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approval_stage_is_present_only_when_confirmation_is_required() {
|
||||
let _tracing_test_guard = TRACING_TEST_LOCK.lock().await;
|
||||
let capture = TraceCapture::default();
|
||||
let subscriber = tracing_subscriber::registry().with(capture.clone());
|
||||
let executor = RuntimeExecutorBuilder::new()
|
||||
@@ -151,6 +194,7 @@ async fn approval_stage_is_present_only_when_confirmation_is_required() {
|
||||
|
||||
#[tokio::test]
|
||||
async fn idempotency_stage_distinguishes_execution_from_replay() {
|
||||
let _tracing_test_guard = TRACING_TEST_LOCK.lock().await;
|
||||
let capture = TraceCapture::default();
|
||||
let subscriber = tracing_subscriber::registry().with(capture.clone());
|
||||
let executor = RuntimeExecutorBuilder::new()
|
||||
@@ -238,6 +282,37 @@ impl ProtocolAdapter for SuccessAdapter {
|
||||
}
|
||||
}
|
||||
|
||||
struct ContextCapturingAdapter {
|
||||
captured_headers: Arc<Mutex<Option<BTreeMap<String, String>>>>,
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl ProtocolAdapter for ContextCapturingAdapter {
|
||||
fn protocol(&self) -> Protocol {
|
||||
Protocol::Rest
|
||||
}
|
||||
|
||||
fn supports_mode(&self, mode: ExecutionMode) -> bool {
|
||||
mode == ExecutionMode::Unary
|
||||
}
|
||||
|
||||
async fn invoke_unary(
|
||||
&self,
|
||||
_target: &Target,
|
||||
_prepared: &crank_core::PreparedRequest,
|
||||
context: &crank_core::RuntimeRequestContext,
|
||||
) -> Result<AdapterResponse, ProtocolAdapterError> {
|
||||
*self.captured_headers.lock().expect("captured headers lock") =
|
||||
Some(context.outbound_headers());
|
||||
Ok(AdapterResponse {
|
||||
status_code: 200,
|
||||
headers: BTreeMap::new(),
|
||||
body: json!({"accepted": true}),
|
||||
data: json!({"accepted": true}),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
fn operation() -> Operation<Schema, MappingSet> {
|
||||
Operation {
|
||||
id: OperationId::new("op_stage_test"),
|
||||
@@ -63,24 +63,13 @@ impl Stage {
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn db_span(self, operation: DbOperation) -> Option<Span> {
|
||||
if self != Self::DbQuery {
|
||||
return None;
|
||||
}
|
||||
let span = self.span();
|
||||
span.record("db.operation", operation.as_str());
|
||||
Some(span)
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn observe_db_query<T, E>(
|
||||
operation: DbOperation,
|
||||
future: impl Future<Output = Result<T, E>>,
|
||||
) -> Result<T, E> {
|
||||
let span = Stage::DbQuery
|
||||
.db_span(operation)
|
||||
.expect("database operation requires db.query stage");
|
||||
let span = operation.span();
|
||||
let result = future.instrument(span.clone()).await;
|
||||
match &result {
|
||||
Ok(_) => StageOutcome::Success.record(&span),
|
||||
@@ -182,6 +171,12 @@ pub enum DbOperation {
|
||||
}
|
||||
|
||||
impl DbOperation {
|
||||
pub fn span(self) -> Span {
|
||||
let span = Stage::DbQuery.span();
|
||||
span.record("db.operation", self.as_str());
|
||||
span
|
||||
}
|
||||
|
||||
pub const fn as_str(self) -> &'static str {
|
||||
match self {
|
||||
Self::MachineAccessRead => "machine_access.read",
|
||||
|
||||
@@ -15,9 +15,7 @@ fn stage_names_and_attributes_are_closed() {
|
||||
ErrorCategory::Mapping.record(&span);
|
||||
drop(span);
|
||||
|
||||
let db_span = Stage::DbQuery
|
||||
.db_span(DbOperation::InvocationHistoryWrite)
|
||||
.expect("db stage accepts a db operation");
|
||||
let db_span = DbOperation::InvocationHistoryWrite.span();
|
||||
StageOutcome::Error.record(&db_span);
|
||||
drop(db_span);
|
||||
});
|
||||
@@ -32,12 +30,8 @@ fn stage_names_and_attributes_are_closed() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_database_stage_rejects_database_attributes() {
|
||||
assert!(
|
||||
Stage::RuntimeExecute
|
||||
.db_span(DbOperation::CatalogLoad)
|
||||
.is_none()
|
||||
);
|
||||
fn database_operation_names_are_closed() {
|
||||
assert_eq!(DbOperation::CatalogLoad.as_str(), "catalog.load");
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
|
||||
Reference in New Issue
Block a user