Add alert metadata #3764
Merged
Add alert metadata #3764
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mastercactapus
requested changes
Mar 21, 2024
Co-authored-by: Nathaniel Caza <[email protected]>
Co-authored-by: Nathaniel Caza <[email protected]>
Co-authored-by: Nathaniel Caza <[email protected]>
Since we're handling this separatly with a specific store method, we don't want to add a second place to set metadata here. If/when we update things using CreateOrUpdate, we'll make a Tx method for it. This way we don't need to update call sites across the app for the new signature.
So this query is a bit more complicated, but we want to make sure no future bug allows a service/integration key to update the metadata of an alert that doesn't belong to it. Integration keys in GoAlert are tied to a single service, and all DB calls regarding alerts should be guarded such that only alerts belonging to that specific service may be modified, including the newly introduced metadata. The approach I took here allows us to pass a service ID, optionally (i.e., ONLY when a request comes from an integration), where the query will fail to insert/update any data if it's mismatched. If the service ID is null, it will skip this check (i.e., a user action).
This is just updating the type from int to bigint (64 bit) and using @alert_ids instead of $1 so that sqlc generates a better name for the argument.
For organization sake, moving the store methods to a separate file (we have a bad habbit of having the store.go files growing forever). I also implemented things using the `gadb` and taking a `DBTX` rather than a sql.Tx. This is the most recent pattern, although most DB code has not been refactored for this. I'm using the `map[string]string` Go type directly to get rid of any indirection on the metadata format external to this package. Internally, I added a `metadataDBFormat` that hopefully by name makes it clearer it's only intended to be used at rest. Compared to what was there before, I also: - Added assertions on the type string - Added a MetadataAlertID type that includes an alert ID for the dataloader - Added an Access Denied error if SetMetadataTx fails because of an invalid alert or service ID mismatch
weefatboi
previously approved these changes
Apr 1, 2024
- removed debug print - removed redundant nil check on Metadata() result - updated Metadata comment regarding return values
mastercactapus
previously approved these changes
Apr 1, 2024
mastercactapus
previously approved these changes
Apr 1, 2024
mastercactapus
previously approved these changes
Apr 1, 2024
mastercactapus
approved these changes
Apr 2, 2024
weefatboi
approved these changes
Apr 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
make checkto catch common errors. Fixed any that came up.Description:
Which issue(s) this PR fixes:
Add Metadata to Alerts for Traceability and Integration
Out of Scope:
Display of metadata in goAlert UI
Screenshots:
n/a
Describe any introduced user-facing changes:
None
Describe any introduced API changes:
Users can add 'meta' field in the create alert requests to track additional metadata(kv pairs)