Emit forwarder version on request (hubmanager version-check) - #164
Emit forwarder version on request (hubmanager version-check)#164erics-ddog wants to merge 2 commits into
Conversation
hubmanager reconciles each forwarder's OCI Function image against Balto's desired tag, but registry/image-cache propagation lag means the control plane reporting "updated" doesn't guarantee a given container is actually running the new code. Stamp the image tag into the binary at build time and emit it as a gauge tagged by forwarder/version/region once per cold start (not per invocation, since the version is fixed for a container's lifetime), so rollout propagation is directly observable in Datadog.
EamonBrady1
left a comment
EamonBrady1
left a comment
There was a problem hiding this comment.
This would be a pretty notable change and is sending a custom metric to Datadog from the customer account (which would charge them). We currently don't have any behavior in the forwarding functions that emits monitoring data directly to Datadog.
Additionally, if we are sending it with this client, it would appear as monitoring data in their account, invisible to us unless we partlow into user accounts every time we want to check this, which we wouldn't want to do.
I think the solution to this issue is a bit more complicated than just emitting a metric from the forwarding functions and is part of bigger picture OCI monitoring that we've thought about in the past. It might require a new endpoint to send this monitoring data to or something like that, but I think we need to think about this one a little more
| var Version = "unknown" | ||
|
|
||
| func main() { | ||
| client.EmitColdStart("logs", Version) |
There was a problem hiding this comment.
Given this is called synchronously, it could take up to 3 minutes and 10 seconds to run which would delay the fdk.Handle call and maybe put it at risk of hitting timeouts elsewhere? It might be good to call the EmitColdStart in a go routine async
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56d31eeced
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var Version = "unknown" | ||
|
|
||
| func main() { | ||
| client.EmitColdStart("events", Version) |
There was a problem hiding this comment.
When Vault or Datadog is slow or unavailable, this synchronous call runs before fdk.Handle in all three forwarders, so the container cannot begin serving. In particular, NewDatadogClientWithSite performs the initial Vault fetch under a three-minute timeout before the later ten-second metric-send timeout is even created; swallowing its eventual error therefore does not make emission non-blocking and can turn a transient observability failure into a failed or severely delayed cold start. Start the FDK first or move the best-effort emission off the startup path.
Useful? React with 👍 / 👎.
| "tags": []string{ | ||
| "forwarder:" + forwarder, | ||
| "version:" + version, | ||
| "region:" + os.Getenv("HOME_REGION"), |
There was a problem hiding this comment.
When a regional stack falls back to the home-region Vault, HOME_REGION is explicitly set to local.vault_region (datadog-integration/modules/regional-stacks/locals.tf:24-32), not to the region where this function executes. Consequently, forwarders running in multiple regions can all report region:<home-region>, collapsing the rollout signal this metric is intended to measure; use an execution-region value or pass the stack's var.region separately.
Useful? React with 👍 / 👎.
| COPY lib/ lib/ | ||
| COPY events-forwarder/ events-forwarder/ | ||
| ARG TARGETARCH | ||
| ARG TAG=unknown |
There was a problem hiding this comment.
The repository's documented local commands in datadog-functions/README.md:10-12 set only the output image tag and do not pass --build-arg TAG=...; Docker does not propagate --tag into this build argument. Anyone following those commands therefore gets a valid tagged image whose binary reports version:unknown, undermining the new metric's claim to identify the running image. Update those build paths to supply TAG or arrange for the version to be derived without a separate required argument.
Useful? React with 👍 / 👎.
Review feedback on the original approach (emitting a metric to Datadog's
intake using the customer's own DD_SITE/API key) correctly flagged that it
would bill the customer for a new custom metric and land in their org,
invisible to Datadog engineers in aggregate. It also ran synchronously before
fdk.Handle, risking cold-start delays if Vault/Datadog were slow.
This replaces the push with a passive echo: forwarders now recognize a
{"version_check":"true"} control message (mirroring the existing
{"backfill_mode":"true"} pattern) and respond with their build-stamped
Version directly from memory — no Vault fetch, no HTTP call, nothing that can
block startup or touch customer credentials. hubmanager (companion change in
dd-source) pulls this by invoking the function synchronously with its own OCI
credentials and emits the comparison as an internal-only metric.
Also fixes the README's local-build instructions to pass --build-arg TAG, so
the manual build path doesn't silently produce a binary reporting
version:unknown.
erics-ddog
changed the title
|
@EamonBrady1 you were right — I've pushed a redesign that removes the push-to-Datadog-intake entirely. Forwarders no longer touch Vault or send anything to the customer's Datadog org for this. Instead they just echo their build-stamped version back when hubmanager invokes them with @bengillmandd this also addresses your synchronous-call concern from a different angle than "make it async": there's no longer any Vault/HTTP call on the cold-start path at all, so nothing can block Also fixed the two Codex findings: region is no longer self-reported via Keeping this as a draft until the hubmanager-side PR is up so they can be reviewed together. |
|
Companion PR is up: https://github.com/ddoghq/dd-source/pull/58292 (hubmanager side — invokes |
| echo "Building and pushing multi-arch image ${IMAGE_PATH} to $REGISTRY..." | ||
| docker buildx build -f ${DOCKER_FILE} \ | ||
| --platform linux/amd64,linux/arm64 \ | ||
| --build-arg TAG="${TAG}" \ |
| out.Write([]byte(`{"status":"error","error":"Internal server error"}`)) | ||
| return | ||
| } | ||
| fdk.WriteStatus(out, 200) |
There was a problem hiding this comment.
do these statuses actually make it to hubmanager? The idea seems right, but I'm curious if this is actually what's surfaced as the function response when we invoke it
3 participants
What
{"version_check":"true"}(mirroring the existing{"backfill_mode":"true"}pattern) and responds with its build-stampedVersiondirectly in the invoke response — no Vault fetch, no HTTP call to Datadog, nothing that touches the customer's Datadog org.Versionis still stamped into the binary at build time via-ldflags "-X main.Version=<tag>"(Dockerfile-events,Dockerfile-logs,Dockerfile-metrics,docker-images.sh), so it reflects what's actually baked into the image, not what OCI's function record claims should be running.fnResponsegains an optionalversionfield; a newwriteVersionResponseanswers the check.--build-arg TAG=<tag>so a locally built image doesn't silently reportversion:unknown.Why (redesign from the original approach)
This PR originally had each forwarder push a
datadog.oci.forwarder.cold_startmetric to Datadog's intake on cold start, using the same client/credentials the forwarder already uses to send the customer's own telemetry. Review feedback (see thread below) correctly caught two real problems with that:fdk.Handle, chained through Vault's 3-minute timeout, so a slow/unavailable Vault or Datadog could delay or fail a cold start over a best-effort observability signal.HOME_REGION, which is the Vault's home region, not necessarily the function's actual execution region in multi-region deployments — collapsing the very rollout signal the metric existed to produce.This redesign flips the direction: instead of the forwarder pushing to Datadog, hubmanager — which already talks to these functions over OCI's control/invoke plane using Datadog's own credentials, and already does exactly this for backfill triggers — pulls the version by invoking the function synchronously and reading the response. See the companion PR in
dd-source: https://github.com/ddoghq/dd-source/pull/58292This resolves all three points above as a side effect: nothing here ever touches customer credentials or the customer's Datadog org (point 1), there is no network/Vault call anywhere on this path so nothing can block
fdk.Handle(point 2), and region tagging moves entirely to hubmanager's side, using its own known-correct region rather than a self-reported env var (point 3).Testing
go build ./.../go vet ./...clean for all three forwarders andlib.go test ./...passes for all three forwarders andlib, including newTestMyHandler_VersionChecktests per forwarder that assert the check succeeds with no environment variables set at all (proving it can't depend on Vault/Datadog config).gofmt -lclean on all changed files.Notes
{"version_check":"true"}synchronously, emits the desired-vs-actual comparison as an internaldd.oci.hubmanager.*metric): https://github.com/ddoghq/dd-source/pull/58292