Skip to content
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

Query and Precompute Non-Contiguous Segments in the Activity Log #15352

Merged
merged 15 commits into from
May 17, 2022

Conversation

HridoyRoy
Copy link
Contributor

This PR:

  1. Changes the query get logic from an algorithm to find the most recent contiguous chunk of segments to finding the closest sub-interval to the user-specified query interval.
  2. Changes the precompute logic to precompute on a minimum of 12 prior segments of activity log data if the number of contiguous segments in storage is less than 12.

@HridoyRoy HridoyRoy requested review from swayne275, hghaf099, pmmukh and a team May 10, 2022 17:02
vault/activity/query.go Outdated Show resolved Hide resolved
@HridoyRoy HridoyRoy requested a review from ccapurso May 16, 2022 18:36
@HridoyRoy HridoyRoy requested a review from ccapurso May 16, 2022 23:11
Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for taking the time to answer my questions!

@HridoyRoy HridoyRoy merged commit 619a8b8 into main May 17, 2022
@HridoyRoy HridoyRoy deleted the vault-6043 branch May 17, 2022 19:17
Copy link
Contributor

@swayne275 swayne275 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this got merged while i was reviewing, but overall i think it looks good. here were the comments i was in the process of leaving

endTime := time.Unix(val, 0).UTC()
s.logger.Trace("end time in consideration is", "end time", endTime, "end time bound", endTimeBound)
if endTime.After(maxEndTime) && !endTime.After(endTimeBound) {
s.logger.Trace("end time has been updated")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you still want this in here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably applies to some of the other Trace() logs too

s.logger.Trace("end time has been updated")
maxEndTime = endTime
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: did you want this newline in here?

closestStartTime := time.Time{}
closestEndTime := time.Time{}
maxTimeDifference := time.Duration(0)
for i := len(filteredList) - 1; i >= 0; i-- {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all nits

  1. is there a reason why we're doing a reverse for loop? i think it might be a bit easier to read with a range loop

  2. do you think it makes sense to break this off into its own separate function? it might make it a bit easier to test this logic, and it also might make it more understandable for future engineers

i only ask these things because activity log is growing fairly hard to dive into and understand for those less familiar

@@ -483,6 +483,23 @@ func (a *ActivityLog) getMostRecentActivityLogSegment(ctx context.Context) ([]ti
return timeutil.GetMostRecentContiguousMonths(logTimes), nil
}

// getMostRecentActivityLogSegment gets the times (in UTC) associated with the most recent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love the UTC callout!

@@ -483,6 +483,23 @@ func (a *ActivityLog) getMostRecentActivityLogSegment(ctx context.Context) ([]ti
return timeutil.GetMostRecentContiguousMonths(logTimes), nil
}

// getMostRecentActivityLogSegment gets the times (in UTC) associated with the most recent
// contiguous set of activity logs, sorted in decreasing order (latest to earliest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does latest to earliest == most to least recent?

Gabrielopesantos pushed a commit to Gabrielopesantos/vault that referenced this pull request Jun 6, 2022
…hicorp#15352)

* query and precompute non-contiguous segments in the activity log

* changelog

* newline formatting

* make fmt

* report listener and storage types as found keys

* report listener and storage types as found keys

* Update vault/activity_log_test.go

Co-authored-by: Chris Capurso <1036769+ccapurso@users.noreply.github.com>

* review comments

* merge conflict

* merge conflict

* merge conflict

* fix unchecked merge conflict

Co-authored-by: Chris Capurso <1036769+ccapurso@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants