Logging salt age stats on rotation #475
Conversation
aulme
force-pushed
the
aul-UID2-5349-logging-salt-stats
branch
3 times, most recently
from
7904e04 to
d8e1775
Compare
aulme
force-pushed
the
aul-UID2-5349-logging-salt-stats
branch
from
d8e1775 to
6ebc73b
Compare
| return minInclusive <= t && t < maxExclusive; | ||
| private boolean isRotatable(long nextEffective, SaltEntry salt) { | ||
| if (this.isRefreshFromEnabled) { | ||
| if (salt.refreshFrom() == null) { // TODO: remove once refreshFrom is no longer optional |
There was a problem hiding this comment.
Is it still optional? I thought we've populated this everywhere now?
There was a problem hiding this comment.
Yeah, in practice we have that populated in every environment, there are still code paths for null case though. I'll remove it from here, we can clean up the others later.
| return Result.fromSnapshot(nextSnapshot); | ||
| } | ||
|
|
||
| private List<Integer> findRotatableSaltIndexes(SaltEntry[] preRotationSalts, long nextEffective) { |
There was a problem hiding this comment.
Is there a reason why we can't use salt bucket IDs rather than indexes?
There was a problem hiding this comment.
could even just pass around arrays of salt entries and override its equal method if that works better
There was a problem hiding this comment.
We could, I wanted to be consistent with how saltIndexesToRotate is done, which is indexes.
I guess that's done for performance reasons, so we can address specific indexes in the array of >1m rather than cycling through it. I can look at how much difference switching to objects across the board would make, might not be enough to matter.
| if (shouldRotate) { | ||
| return salt.currentSalt(); | ||
| } | ||
| long age = nextEffective - salt.lastUpdated(); |
There was a problem hiding this comment.
Should we add a SaltEntry.ageInDays(asOf) method?
There was a problem hiding this comment.
SaltEntry is in Shared and currently this isn't needed anywhere except Admin. I guess we might need this for Operator though so might be ok.
There was a problem hiding this comment.
I think Operator checks salt age as well, so would be broadly useful
There was a problem hiding this comment.
We could just have one Admin helper function for this if we don't want to add it to shared
| } | ||
|
|
||
| for (var entry : ages.entrySet()) { | ||
| LOGGER.info("{} target-date={} age={} salts={}", logEvent, formattedDate, entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
perhaps:
salts={} target-date={} age={} num_salts={}
There was a problem hiding this comment.
What's salts vs num_salts here? logEvent?
There was a problem hiding this comment.
salts would be the set of salts "rotatable-salts", "rotated-salts", etc. How were you planning on identifying the set of salts without a logfmt label, regex?
num_salts would be the number of salts
| } | ||
|
|
||
| private void logSaltCounts(LocalDate targetDate, Instant nextEffective, SaltEntry[] preRotationSalts, SaltEntry[] postRotationSalts, List<Integer> rotatableSaltIndexes, List<Integer> rotatedSaltIndexes) { | ||
| var rotatableSalts = onlySaltsAtIndexes(preRotationSalts, rotatableSaltIndexes); |
There was a problem hiding this comment.
Can we pass in rotatableSalts, rotatedSalts (and saltsForRefresh) directly into this function?
There was a problem hiding this comment.
RotatableSalts are saltsForRefresh
aulme
force-pushed
the
aul-UID2-5349-logging-salt-stats
branch
from
2d71626 to
7dbead3
Compare
aulme
force-pushed
the
aul-UID2-5349-logging-salt-stats
branch
2 times, most recently
from
abc1d53 to
64eac21
Compare
aulme
force-pushed
the
aul-UID2-5349-logging-salt-stats
branch
from
64eac21 to
27e005b
Compare
| if (shouldRotate) { | ||
| return salt.currentSalt(); | ||
| } | ||
| if (targetDate.saltAgeInDays(salt) < 90) { |
There was a problem hiding this comment.
should this be less than or equal to? or less than?
There was a problem hiding this comment.
I think this should be <, not <= since this includes the day on which we actually rotated the salt when the age would be 0. This results in people seeing previousSalt for 90 days exactly.
The current behaviour is also less than, we're even testing for it in L295.
No description provided.