metrics: activity log #10514
metrics: activity log #10514
Conversation
|
@mgritter I've added I'm marking this ready-for-review just to get some feedback if these are what you expected. |
vault/activity_log.go
Outdated
| endTime := timeutil.EndOfMonth(time.Unix(lastMonth, 0).UTC()) | ||
| activePeriodStart := endTime.AddDate(0, -a.defaultReportMonths, 0) |
There was a problem hiding this comment.
This unfortunately doesn't work as expected. Suppose endTime is October 31, then subtracting 8 months gives not Feb 28th or 29th but "Feb 31th" which becomes March 3 or 2.
timeutil.MonthsPreviousTo should do what you need here, though it picks the start of the month. (The same problem does not arise if using the 1st of the month.)
vault/activity_log.go
Outdated
| for nsID, counts := range byNamespace { | ||
| pq.Namespaces = append(pq.Namespaces, &activity.NamespaceRecord{ | ||
| NamespaceID: nsID, | ||
| Entities: uint64(len(counts.Entities)), | ||
| NonEntityTokens: counts.Tokens, | ||
| }) | ||
|
|
||
| if startTime.After(activePeriodStart) { |
There was a problem hiding this comment.
This could fire for every previous month in our retention period, not just once at defaultReportMonths.
If you use the utility function suggested above, that should allow an equality check instead... except for the potential partial month (if the feature was enabled on Jan 15, then that first startTime will be Jan 15, not Jan 1.)
So, I would suggest using timeutil.InRange( startTime, activePeriodStart, EndOfMonth( activePeriodStart )) or writing a new utility in timeutil that does that.
vault/activity_log.go
Outdated
| // emit active entity metrics for the interval (times[0], endTime) | ||
| for nsID, counts := range byNamespace { | ||
| a.metrics.SetGaugeWithLabels( | ||
| []string{"identity", "entity", "active", "monthly"}, |
There was a problem hiding this comment.
The comment is correct but I don't believe that is what this code does
Because we've finished the loop, byNamespace contains all the data from the interval (times[len(times)-1], endTime). times is in reverse order with 0 being the most recent month.
A check at the same place you're doing the defaultReportingPeriod check would be sufficient.
The storage size metric is the one I thought might be infeasible. |
|
This pull request is being automatically deployed with Vercel (learn more). vault – ./website🔍 Inspect: https://vercel.com/hashicorp/vault/bjy5xmm9w [Deployment for 16fdccd failed] vault-storybook – ./ui🔍 Inspect: https://vercel.com/hashicorp/vault-storybook/1hq7d9y6f [Deployment for 5c632ef canceled] |
Updated documentation.
No description provided.