-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement verification cache #3801
Merged
+546
−36
Merged
Changes from all commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
6f54591
initial sketch
rosecodym 0a9a30b
allow forced cache misses
rosecodym 0f0917b
rearrange
rosecodym d074f6d
plug into engine
rosecodym 6e9d760
clear stuff in cached copy
rosecodym 61a70a3
fiddle with pointers more
rosecodym 3e5ff9c
rename to verificationCache
rosecodym 5ac0139
inject getCacheKey
rosecodym a4e092c
optimize when forcing a cache miss
rosecodym 8d83daa
tweak
rosecodym fd58380
tweak more
rosecodym c119f2b
flag when cache was used
rosecodym 15ddd63
store key builder in engine
rosecodym ea3a559
rename
rosecodym bf3170d
copy verification errors
rosecodym 3229f3b
Merge remote-tracking branch 'origin/main' into detection-caching-pla…
rosecodym b126569
re-remove decodertype
rosecodym 8ef29ec
remove cached decoder type
rosecodym 193f5fa
Merge remote-tracking branch 'origin/main' into detection-caching-pla…
rosecodym 20aa4ea
add tests and fix bugs
rosecodym 328e121
tweak formatting
rosecodym 817ea15
update engine
rosecodym ca07df2
calculate cache key by value
rosecodym e2924de
update comment
rosecodym 3d097de
Merge remote-tracking branch 'origin/main' into detection-caching-pla…
rosecodym d5f8f31
add janky metrics + an actual cache
rosecodym abaa8dc
fix typo
rosecodym e950bc9
add flag
rosecodym add0c0a
add to printers
rosecodym 076696a
add new metric
rosecodym 48d9535
create struct
rosecodym 8482b4d
rewrite metrics
rosecodym 2d21f7b
create ResultCache type
rosecodym c7390d4
use blake2b instead of configurable cache key
rosecodym 11e7906
add comments
rosecodym 7040b4d
report durations
rosecodym 572cd1b
re-indent
rosecodym ce19d5a
rename package
rosecodym ad44a84
add thread safety comment
rosecodym e7d7a01
clarify comment
rosecodym 20702c5
Merge remote-tracking branch 'origin/main' into detection-caching
rosecodym f90530c
test nil metrics
rosecodym 52997eb
check for nil metrics and add more comments
rosecodym 6ba1339
tweak comment
rosecodym c34f40a
stop copying mutexes derp
rosecodym 548c4dc
i hate computers
rosecodym 4ad471a
they're so bad
rosecodym File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package verificationcache | ||
|
||
import ( | ||
"sync/atomic" | ||
"time" | ||
) | ||
|
||
// InMemoryMetrics is a MetricsReporter that stores reported metrics in memory for retrieval at the end of a scan. | ||
type InMemoryMetrics struct { | ||
CredentialVerificationsSaved atomic.Int32 | ||
FromDataVerifyTimeSpentMS atomic.Int64 | ||
ResultCacheHits atomic.Int32 | ||
ResultCacheHitsWasted atomic.Int32 | ||
ResultCacheMisses atomic.Int32 | ||
} | ||
|
||
var _ MetricsReporter = (*InMemoryMetrics)(nil) | ||
|
||
func (m *InMemoryMetrics) AddCredentialVerificationsSaved(count int) { | ||
m.CredentialVerificationsSaved.Add(int32(count)) | ||
} | ||
|
||
func (m *InMemoryMetrics) AddFromDataVerifyTimeSpent(wallTime time.Duration) { | ||
m.FromDataVerifyTimeSpentMS.Add(wallTime.Milliseconds()) | ||
} | ||
|
||
func (m *InMemoryMetrics) AddResultCacheHits(count int) { | ||
m.ResultCacheHits.Add(int32(count)) | ||
} | ||
|
||
func (m *InMemoryMetrics) AddResultCacheMisses(count int) { | ||
m.ResultCacheMisses.Add(int32(count)) | ||
} | ||
|
||
func (m *InMemoryMetrics) AddResultCacheHitsWasted(count int) { | ||
m.ResultCacheHitsWasted.Add(int32(count)) | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package verificationcache | ||
|
||
import "time" | ||
|
||
// MetricsReporter is an interface used by a verification cache to report various metrics related to its operation. | ||
// Implementations must be thread-safe. | ||
type MetricsReporter interface { | ||
// AddCredentialVerificationsSaved records "saved" verification attempts, which is when credential verification | ||
// status is loaded from the cache instead of retrieved from a remote verification endpoint. This number might be | ||
// smaller than the cache hit count due to cache hit "wasting"; see AddResultCacheHitsWasted for more information. | ||
AddCredentialVerificationsSaved(count int) | ||
|
||
// AddFromDataVerifyTimeSpent records wall time spent in calls to detector.FromData with verify=true. | ||
AddFromDataVerifyTimeSpent(wallTime time.Duration) | ||
|
||
// AddResultCacheHits records result cache hits. Not all cache hits result in elided remote verification requests | ||
// due to cache hit "wasting"; see AddResultCacheHitsWasted for more information. | ||
AddResultCacheHits(count int) | ||
|
||
// AddResultCacheMisses records result cache misses. | ||
AddResultCacheMisses(count int) | ||
|
||
// AddResultCacheHitsWasted records "wasted" result cache hits. A "wasted" result cache hit is a result cache hit | ||
// that does not elide a remote verification request because there are other secret findings in the relevant chunk | ||
// that are not cached. When this happens, the detector's FromData method must be called anyway, so the cache hit | ||
// doesn't save any remote requests. | ||
AddResultCacheHitsWasted(count int) | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package verificationcache | ||
|
||
import ( | ||
"github.com/trufflesecurity/trufflehog/v3/pkg/cache" | ||
"github.com/trufflesecurity/trufflehog/v3/pkg/detectors" | ||
) | ||
|
||
// ResultCache is a cache that holds individual detector results. It serves as a component of a VerificationCache. | ||
type ResultCache cache.Cache[detectors.Result] |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: @rosecodym any reason we need this to be an interface? I'm seeing
InMemoryMetrics
as the only struct that implements this interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good question. The plan is to use this seam to plug in a Prometheus-based reporter in enterprise. Rather than drape those metrics all over the OSS codebase I opted to create this interface, which looks silly on its own (as you've pointed out) but I hope will save some headaches in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have prometheus metrics in the OSS codebase elsewhere, it doesn't cause any issues.