Add outcome tag to upload_cert and log skipped IAM cert uploads - #364
Add outcome tag to upload_cert and log skipped IAM cert uploads#364maperu wants to merge 1 commit into
Conversation
61dcae8 to
2de8726
Compare
maperu
changed the title
![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: 2de8726d65
ℹ️ 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".
IAM server-certificate names are unique per account regardless of path, so a cert shared between a CloudFront destination (/cloudfront/) and an IAM destination (/) collides on the second upload with EntityAlreadyExists, which upload_cert swallowed silently. Log an error and record the outcome on that path so the no-op is observable. Reshape the existing upload_cert counter: drop the high-cardinality name tag (unqueryable in Datadog) and add a low-cardinality outcome tag (success / skipped_already_exists), emitted once the result is known. The emit is wrapped so a telemetry error can never escape into the @Retry wrapper and re-run the non-idempotent upload. No dashboard or monitor references the old tag shape.
2de8726 to
cb68f33
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb68f3373c
ℹ️ 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".
| metrics.send( | ||
| "upload_cert", "counter", 1, metric_tags={"path": path, "outcome": outcome} | ||
| ) |
There was a problem hiding this comment.
When IAM destinations in different physical locations use the same path (commonly /), these new success/skip series are aggregated because the metric includes only path and outcome; in particular, a skipped upload cannot be associated with the affected datacenter. Thread the destination description through AWSDestinationPlugin.upload and add its datacenter value as required for metrics describing certificate deployments.
AGENTS.md reference: AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. There's already a destination_upload counter in certificates/models.py that fires right after this upload and carries datacenter
| "upload_cert", "counter", 1, metric_tags={"path": path, "outcome": outcome} | ||
| ) | ||
| except Exception: | ||
| current_app.logger.exception("Failed to emit upload_cert metric") |
There was a problem hiding this comment.
| current_app.logger.exception("Failed to emit upload_cert metric") | |
| current_app.logger.exception(f"Failed to emit upload_cert metric, path: {path}, outcome {outcome}") |
| except botocore.exceptions.ClientError as e: | ||
| if e.response["Error"]["Code"] != "EntityAlreadyExists": | ||
| raise e |
There was a problem hiding this comment.
This means we dont have any metric coverage for exceptions outside of skipped_already_exists and success
2 participants
When a certificate is shared between a CloudFront destination (uploaded to IAM under
/cloudfront/) and a plain IAM destination (uploaded under/), the second upload hitsEntityAlreadyExistsbecause IAM server-certificate names are unique per account regardless of path.upload_certcaught that error and returned silently, so the skipped destination upload left no log line and no distinct metric. That is exactly the shape behind CLOUDR-2050, and there was no telemetry to notice it happening.This adds a warning log on that branch so the no-op shows up in the logs with the cert name and path.
It also reshapes the existing
upload_certcounter instead of adding a parallel metric. The old emit fired before the upload with a high-cardinalitynametag (per cert, so unqueryable in Datadog) and could not distinguish a success from a skip. Now the metric emits once the outcome is known, with thenametag dropped and a low-cardinalityoutcometag added (successorskipped_already_exists), keepingpath. I checked the Lemur dashboard and the Lemur monitors: nothing referenceslemur.upload_certor itsnametag, so reshaping it is safe.