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

VAULT-17555: Secret sync client metrics #25713

Merged
merged 5 commits into from
Mar 1, 2024

Conversation

miagilepner
Copy link
Contributor

This PR refactors the metrics reporting from the activity log to move it to a separate function. I've also added a test and two new metrics for secret sync.

The documentation for the new metrics will be in a separate PR, along with the rest of the secret sync client count documentation.

@miagilepner miagilepner added this to the 1.16.0-rc3 milestone Feb 29, 2024
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Feb 29, 2024
@miagilepner miagilepner requested a review from a team February 29, 2024 13:44
Copy link

github-actions bot commented Feb 29, 2024

CI Results:
All Go tests succeeded! ✅

Copy link

github-actions bot commented Feb 29, 2024

Build Results:
All builds succeeded! ✅

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM to the extent that I understand the details of secret sync and client counts and this code in particular (which is not very large!). I don't see any obvious logical errors or issues other than the optional bike-shed comment inline!

Comment on lines 2374 to 2377
a.metrics.SetGauge([]string{"identity", strings.ReplaceAll(clientType, "-", ""), "active", "monthly"}, float32(count))
}
for clientType, count := range summedMetricsReporting {
a.metrics.SetGauge([]string{"identity", strings.ReplaceAll(clientType, "-", ""), "active", "reporting_period"}, float32(count))
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider substituting _ for rather than removing hyphens? I think our metrics often (though probably not consistentlty) use underscores for multi-word terms -- for example reporting_period in the same metric name here. I have a very gentle preference towards identity.non_entity.active.reporting_period rather than identity.nonentity.active.reporting_period on consistency grounds. But not a big deal if you disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also prefer underscores. We already have the metric nonentity and so I was trying to make secret syncs follow the same pattern, but there's no reason why it needs to. I'll change it.

@miagilepner miagilepner merged commit ac1347b into main Mar 1, 2024
83 checks passed
@miagilepner miagilepner deleted the miagilepner/activity-secret-sync-metrics branch March 1, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants