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

RA: Count new registrations with contacts #7984

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

beautifulentropy
Copy link
Member

Adding a temporary metric to estimate the rate of new accounts that include a contact.

Part of #7966

@beautifulentropy beautifulentropy requested a review from a team as a code owner January 29, 2025 15:13
@beautifulentropy beautifulentropy requested a review from jsha January 29, 2025 15:13
aarongable
aarongable previously approved these changes Jan 29, 2025
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM. Might want to also increment the metric (perhaps with a different label) in the updateAccount codepath, right before calling ra.UpdateRegistrationContact.

@beautifulentropy beautifulentropy force-pushed the ra-count-registrations-with-contacts branch from 7e08690 to d8ab760 Compare January 29, 2025 18:59
@beautifulentropy
Copy link
Member Author

beautifulentropy commented Jan 29, 2025

LGTM. Might want to also increment the metric (perhaps with a different label) in the updateAccount codepath, right before calling ra.UpdateRegistrationContact.

I’ve actually slightly improved the implementation after noticing that the number of contacts permitted is variable and I chose to increment inside of UpdateRegistrationContact and explicitly after ra.validateContacts has returned without error. We only want to include contacts that we would actually be willing to dispatch.

Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

What's the purpose of having both "new" and "update" labels, when they are always set to have opposite values?

aarongable
aarongable previously approved these changes Jan 30, 2025
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM with two minor notes:

  • Feels a little odd to do the metric increment after writing to the SA in NewRegistration, but do it before writing to the SA in UpdateRegistrationContact. Makes readers question if there's a reason for the inconsistency. Sorry I didn't notice this on first pass.
  • Doing this after ra.validateContacts is great for now, but I don't necessarily want to set the precedent that we'll be validating contacts in boulder before exporting them. I think that other systems are much better equipped to handle that than we are (beyond basic basic sanity checks). So far I've been imagining that in the future, contacts will be exported from the WFE, and never make it to the RA at all.

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Since this is now counting "number of contacts received" rather than "number of registrations with contacts," the metric name and PR summary should be updated to match.

@beautifulentropy
Copy link
Member Author

beautifulentropy commented Jan 31, 2025

Since this is now counting "number of contacts received" rather than "number of registrations with contacts," the metric name and PR summary should be updated to match.

Hah, good call. I hadn't noticed the incongruity post-change.

aarongable
aarongable previously approved these changes Jan 31, 2025
ra/ra.go Outdated Show resolved Hide resolved
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.

3 participants