[Serverless-Init] Support Cloud Run Jobs with serverless-init#38737
Conversation
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: f4aa22d Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +2.82 | [-0.24, +5.88] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +2.82 | [-0.24, +5.88] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.94 | [+0.56, +1.33] | 1 | Logs bounds checks dashboard |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +0.82 | [+0.76, +0.89] | 1 | Logs |
| ➖ | docker_containers_memory | memory utilization | +0.40 | [+0.32, +0.47] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | +0.26 | [+0.14, +0.38] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | +0.20 | [+0.17, +0.24] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | +0.20 | [+0.16, +0.24] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.09 | [+0.05, +0.13] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.05 | [-0.52, +0.61] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.02 | [-0.51, +0.55] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.01 | [-0.58, +0.60] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.01 | [-0.30, +0.31] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.02, +0.02] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.00 | [-0.59, +0.59] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | -0.00 | [-0.15, +0.15] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | -0.01 | [-0.13, +0.12] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.06 | [-0.10, -0.02] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | -0.06 | [-0.15, +0.03] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -2.37 | [-5.10, +0.37] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | 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_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 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | bounds checks dashboard |
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_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.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check lost_bytes: 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_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
|
| // GetTags returns a map of gcp-related tags for Cloud Run Jobs. | ||
| func (c *CloudRunJobs) GetTags() map[string]string { | ||
| tags := metadataHelperFunc(GetDefaultConfig(), false) | ||
| tags["origin"] = CloudRunJobsOrigin |
There was a problem hiding this comment.
I think this can be removed. We wanted to make origin a hidden tag, but in case some customers were using it, we needed to be backward compatible. Since this is a new product, we can exclude it here
There was a problem hiding this comment.
good call, thanks for catching this!
|
|
||
| metric.AddShutdownMetric(prefix, origin, metricAgent.GetExtraTags(), time.Now(), metricAgent.Demux) | ||
| // Don't emit shutdown metric for Cloud Run Jobs | ||
| if _, ok := cloudService.(*cloudservice.CloudRunJobs); !ok { |
There was a problem hiding this comment.
nit: this sort of thing is usually a smell, adding service-specific logic gated by an if statement checking the service's struct type.
can we move this logic into the cloudService class, with the cloud run job implementation as a noop? or at the very least, can we add a shouldEmitShutdownMetric method to the cloud run services so that we consolidate the service-specific logic in with the cloud service implementation?
(i know we have other things like this already. i think i've seen some azure-specific code sprinkled around, but let's not propagate unfortunate patterns if we don't have to.)
There was a problem hiding this comment.
actually, not a nit. let's do this differently. this and the cold start metric as well.
There was a problem hiding this comment.
We emit this enhanced metric for Azure and GCP. I agree we can move this into cloud service, but we were also talking about adding cloud run job specific enhanced metrics relating to the jobs and tasks, like AWS step functions. I'll leave it up to Nick if he wants to do this in a separate PR or in this one
There was a problem hiding this comment.
Yes, since we're going to emit an enhanced metric, we can rename AddColdStartMetric to AddStartMetric, which will be implemented by each service. Same with shutdown. wdyt?
apiarian-datadog
left a comment
There was a problem hiding this comment.
looks fine to me!
| return &CloudRun{spanNamespace: cloudRunService} | ||
| } | ||
|
|
||
| if isCloudRunJob() { |
There was a problem hiding this comment.
probably not in this pr, but this whole if ladder is a bit funky since it leaks the individual service details into this overall interface. might be nice to refactor this. but if you're not going to be making any more changes, maybe add a note for us to do it in the future?
There was a problem hiding this comment.
i agree, that's wonky. left a comment
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What does this PR do?
Support Cloud Run Jobs in serverless-init. So we get a variety of features:
cloudrunAdded logic to skip cold start & shutdown metrics, since they're irrelevant for Cloud Run jobs (every execution will have a "cold start", every execution will have a shutdown).
I follow the GCP convention and prefix logs with
gcp.run.jobinstead ofgcp.run.Motivation
Describe how you validated your changes
Tested manually and unit tests
Possible Drawbacks / Trade-offs
Additional Notes
Jobs kind of work with the sidecar approach, but the flushing logic is sketchy. When the main app is done executing, all other containers immediately receive a
SIGKILL, so any pending traces or logs will not be sent.Therefore, we will be recommending customers use the in-process approach in our documentation.