Settings: log set operations #3194
Merged
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
cbgbt
reviewed
Jul 1, 2023
sources/api/datastore/src/lib.rs
Outdated
Comment on lines
154
to
156
| _ => { | ||
| trace!("Updating data key {}", key.name()); | ||
| } |
There was a problem hiding this comment.
It seems like the Committed enum is used to describe the state of a transaction rather than whether or not the write is a set/update. Is this right?
There was a problem hiding this comment.
You probably know our datastore operations better than I do. Are there any cases where we would be committing something that would not be a set/update?
webern
reviewed
Jul 5, 2023
This adds info level logging to capture some trace of setting updates in the system log at the default logging level. This is to help capture a record of system changes being made. This is only a baby step towards getting better system modification auditing in place, but it is a very minor change that can be done now until a more comprehensive plan can be put in place. Signed-off-by: Sean McGinnis <[email protected]>
bcressey
approved these changes
Jul 10, 2023
webern
approved these changes
Jul 12, 2023
zmrow
approved these changes
Jul 12, 2023
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.
Issue number:
Related #2640
Description of changes:
This adds info level logging to capture some trace of setting updates in the system log at the default logging level. This is to help capture a record of system changes being made.
This is only a baby step towards addressing #2640, but it is a very minor change that can be done now until a more comprehensive plan can be put in place.
This logs updates to individual keys, even if a set of values are being updated in one call. This does cause a slew of log messages on system start up as the initial datastore is being created. This is relatively small though, and is useful in seeing what all initial settings are being set on initial boot. After that point, only individually set values should show up in the logs, and it is easy to see when settings changes have been made.
A more comprehensive audit log should be put in place, but that will require handling to make sure no sensitive data (private keys, passwords, etc) make it to the log. There are also likely other operations besides
set_settingthat should be captured. But this is a trivial change that at least gets some breadcrumbs to be able to monitor for changes.Testing done:
Deployed an instance and verified updates were reflected in the system log.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.