Skip to content

Add support for Transfer-Encoding header for GCP Go Gen 1 Cloud Functions #18

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

Merged
nina9753 merged 8 commits into from
May 27, 2025
Merged

Add support for Transfer-Encoding header for GCP Go Gen 1 Cloud Functions #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bcf83cf
Add support for Transfer-Encoding header for GCP Go Gen 1 Cloud Funct…
nina9753 May 19, 2025
dc674ec
update tests
nina9753 May 19, 2025
b34bd4f
update format
nina9753 May 19, 2025
ab28372
revert cargo
nina9753 May 22, 2025
86f0544
update LICENSE-3rdparty.csv
nina9753 May 22, 2025
db14b97
add debug log
nina9753 May 23, 2025
ddeb51d
cargo fmt
nina9753 May 23, 2025
b22beb7
remove log::debug
nina9753 May 23, 2025
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
18 changes: 14 additions & 4 deletions crates/datadog-trace-agent/src/http_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ pub fn log_and_create_traces_success_http_response(
.body(hyper_migration::Body::from(body))
}

/// Takes a request's header map, and verifies that the "content-length" header is present, valid,
/// and less than the given max_content_length.
/// Takes a request's header map, and verifies that the "content-length" and/or "Transfer-Encoding" header
/// is present, valid, and less than the given max_content_length.
///
/// Will return None if no issues are found. Otherwise logs an error (with the given prefix) and
/// returns and HTTP Response with the appropriate error status code.
Expand All @@ -69,8 +69,17 @@ pub fn verify_request_content_length(
let content_length_header = match header_map.get(header::CONTENT_LENGTH) {
Some(res) => res,
None => {
if let Some(transfer_encoding_header) = header_map.get(header::TRANSFER_ENCODING) {
debug!(
"Transfer-Encoding header is present: {:?}",
transfer_encoding_header
);
return None;
}
return Some(log_and_create_http_response(
&format!("{error_message_prefix}: Missing Content-Length header"),
&format!(
"{error_message_prefix}: Missing Content-Length and Transfer-Encoding header"
),
StatusCode::LENGTH_REQUIRED,
));
}
Expand Down Expand Up @@ -134,7 +143,8 @@ mod tests {
assert_eq!(response.status(), StatusCode::LENGTH_REQUIRED);
assert_eq!(
get_response_body_as_string(response).await,
"{\"message\":\"Test Prefix: Missing Content-Length header\"}".to_string()
"{\"message\":\"Test Prefix: Missing Content-Length and Transfer-Encoding header\"}"
.to_string()
);
}

Expand Down
8 changes: 8 additions & 0 deletions crates/datadog-trace-agent/src/trace_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ impl TraceProcessor for ServerlessTraceProcessor {
}
};

// double check content length is < max request content length in case transfer encoding is used
if body_size > config.max_request_content_length {
return log_and_create_http_response(
&format!("Error processing traces: Payload too large"),
StatusCode::PAYLOAD_TOO_LARGE,
);
}

Comment on lines +99 to +106
Copy link
Copy Markdown
Collaborator

@duncanpharvey duncanpharvey May 20, 2025

Choose a reason for hiding this comment

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

What do you think about adding this check inside the verify_request_content_length method? It would be consistent with the rest of the return statements in that method giving an HttpResponse.

Copy link
Copy Markdown
Contributor Author

@nina9753 nina9753 May 22, 2025

Choose a reason for hiding this comment

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

I would prefer not to, as that would require me to update other functions like StatsProcessor::process_stats, which doesn't generate the request size outside of verify_request_content_length. I’d also need to update functions in several spots where including the request length might not be as important. Please feel free to correct me if I’m mistaken, but I thought that we were mainly concerned about the content length for submitting the traces.

Copy link
Copy Markdown
Collaborator

@duncanpharvey duncanpharvey May 23, 2025

Choose a reason for hiding this comment

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

I see what you're saying. If we wanted to make this check inside verify_request_content_length we would need to change the signature of the method to accept body_size, which affects other calls to it (like from StatsProcessor:process_stats. I think it would be preferable to have all of the content length checks in the method for that (verify_request_content_length) but if we only want to make this check for traces then I think your current approach is the best option.

let payload = match trace_utils::collect_pb_trace_chunks(
traces,
&tracer_header_tags,
Expand Down
Loading