Publish metrics for storage drivers types in worker heartbeat#9431
Publish metrics for storage drivers types in worker heartbeat#9431simvlad wants to merge 2 commits intotemporalio:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35ae7d77a5
ℹ️ 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".
common/dynamicconfig/constants.go
Outdated
| ) | ||
| MatchingStorageDriverMetricsAllowedTypes = NewGlobalTypedSetting( | ||
| "matching.storageDriverMetricsAllowedTypes", | ||
| []string{"s3"}, |
There was a problem hiding this comment.
Use nil default for typed allowlist
NewGlobalTypedSetting values are decoded by merging over the default, and dynamicconfig explicitly warns that non-nil slice defaults get appended rather than replaced. With []string{"s3"} as the default here, operators cannot fully override the allowlist (for example, setting [] or [“gcs”] will still keep s3), so storage-driver sanitization and cardinality controls become inaccurate in production configs.
Useful? React with 👍 / 👎.
What changed?
Publish metrics for storage driver types that are passed from SDK through worker heartbeat. This is only enabled if ExternalPayloadsEnabled is set to true. Since drivers can be implemented by the user, we sanitize the list against StorageDriverMetricsAllowedTypes. If the driver name is not present in the list we log it as "other".
Why?
SDK sends the storage driver types used for storing large external payloads. We'd like to publish them as metrics in order to gauge the usage.
How did you test it?
Potential risks
High metric cardinality, but StorageDriverMetricsAllowedTypes restricts the cardinality.