apm: Always set content-type even when we reject a payload #34066
Conversation
ajgajg1134
added
qa/done
github-actions
bot
added
the
short review
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv aws.create-vm --pipeline-id=55991337 --os-family=ubuntuNote: This applies to commit 09b07cd |
Uncompressed package size comparisonComparison with ancestor Diff per package
Decision✅ Passed |
Static quality checks ✅Please find below the results from static quality gates Successful checksInfo
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 4577817 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +1.18 | [-1.88, +4.24] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +0.35 | [-0.56, +1.26] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | +0.20 | [-0.26, +0.66] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.02, +0.01] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | -0.00 | [-0.71, +0.70] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | -0.00 | [-0.92, +0.91] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | -0.01 | [-0.65, +0.62] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.01 | [-0.31, +0.28] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http2 | egress throughput | -0.04 | [-0.90, +0.81] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http1 | egress throughput | -0.06 | [-0.98, +0.87] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | -0.17 | [-0.94, +0.61] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.19 | [-0.96, +0.58] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.29 | [-0.34, -0.25] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.31 | [-0.40, -0.22] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.40 | [-0.45, -0.35] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.56 | [-0.60, -0.52] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | |
| ✅ | quality_gate_logs | memory_usage | 10/10 |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
| // and sending the configured status. | ||
| case r.recvsem <- struct{}{}: | ||
| case <-time.After(time.Duration(r.conf.DecoderTimeout) * time.Millisecond): | ||
| log.Debugf("trace-agent is overwhelmed, a payload has been rejected") |
There was a problem hiding this comment.
This probably should be a Warn or errror (can be ratelimited not to overwhelm the agent with extra processing
There was a problem hiding this comment.
I'm not sure about increasing the severity of this log. This is a somewhat normal behavior, and having it as warn/error may be a bit noisy, even if rate limited. I think the already existing datadog.trace_agent.receiver.payload_refused metric is better to monitor issues.
There was a problem hiding this comment.
I agree with @ichinaski here. @pawelchcki I agree that this agent condition should be more easily seen, but I don't believe increased severity logs are the right approach at this exact moment. With the already existing metric we emit we do have visibility inside orgs when this is happening, and we might want to set up out-of-the-box monitors for these metrics to alert customers (tbd exactly how, I've been having some early conversations with product about this).
I don't think these changes should block this PR though as this behavior already exists, this change is just fixing a bug in that behavior. We will definitely have additional conversations / plans on how to improve this holistically though too. I'm happy to setup a quick meeting too if you feel strongly here about these changes on this PR.
| // this payload can not be accepted | ||
| io.Copy(io.Discard, req.Body) //nolint:errcheck | ||
| switch v { |
There was a problem hiding this comment.
additionally we must introduce new telemetry metric which will allow us to track these errors across orgs.
There was a problem hiding this comment.
not sure if you are referring to telemetry around rejected payloads, but this already exists a few lines below:
r.tagStats(v, req.Header, "").PayloadRefused.Inc()This results in the metric datadog.trace_agent.receiver.payload_refused, which is available in the OOTB Trace Agent dashboard, as well as accessible in the metrics explorer.
github-actions
bot
added
medium review
There was a problem hiding this comment.
After taking a 2nd look - I suggested two changes which should help make this problem more easy to see:
-
change log level from debug to Info or even Higher (this absolutely abnormal operation, if the traces are silently being dropped - the agent effectively ceases to function).
-
add metric to track this across orgs - we must know the scale of this problem (same is true for any other reason we start droping the traces)
|
/merge |
|
View all feedbacks in Devflow UI.
The median merge time in
|
What does this PR do?
Set the correct content-type header for rejected payloads and add a debug log for overwhelmed trace-agents.
Motivation
We should tell tracers the correct content type in our responses. Due to setting the status code before calling replyOK we were inadvertently losing the content type and therefore client's expected plain/text type. This caused some tests to fail internally when the trace-agent became overwhelmed in a test and the PHP tracer failed to parse the sampling rates as the content-type was wrong.
The log is added as it would have made this investigation quite a bit faster, letting us know right away where to start looking.
Describe how you validated your changes
I added a specific unit test for this exact behavior (writing this test taught me about the httptest
Resultmethod which is critical to making this test pass and truly validating the behavior.Possible Drawbacks / Trade-offs
Extra debug log noise? but I believe this trade-off is well worth it.
Additional Notes