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

Document logging best practices #7166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Document logging best practices #7166

wants to merge 1 commit into from

Conversation

aarongable
Copy link
Contributor

No description provided.

@aarongable aarongable requested a review from a team as a code owner November 16, 2023 19:24
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.

We currently violate this rubric to log certain errors (at non-audit level). For instance logDNSError, and certain sampled logging in the OCSP responder.

I think the simplest thing to do is add "3. At most one log line per error". If we decide that gives us too many logs we can later refine the rules for when to log errors.

@aarongable
Copy link
Contributor Author

I feel like logging in the OCSP responder falls under "one log line per request"; the OCSP responder is just so high-volume that we also have to sample there.

logDNSError similarly falls under the "work that happens asynchronously" exception I discuss in the last paragraph. But also it's not clear to me why we log that line at all: wouldn't it be better to include that level of detail in the single "validation failed" audit log object that the VA produces?

I'm fairly opposed to "at most one log line per error" because we have plenty of codepaths that can encounter multiple errors and keep going (or keep retrying). I think the goal I'm trying to accomplish here is to lay out guidelines for us to think about when we're writing new code or making big changes, not to document exactly how things look today.

@jsha
Copy link
Contributor

jsha commented Nov 27, 2023

I'm fairly opposed to "at most one log line per error" because we have plenty of codepaths that can encounter multiple errors and keep going (or keep retrying).

It sounds like you're arguing against "at least one log line per error," right? "At most" means those codepaths are fine.

But also it's not clear to me why we log that line at all: wouldn't it be better to include that level of detail in the single "validation failed" audit log object that the VA produces?

Yes. FWIW we introduced this log line specifically to debug some weird issues we were seeing (id mismatch) and it could probably be removed. The reason it's not logged by the VA is that the information it logs is not part of the interface exposed by the DNS package. It includes things like the encoded query and encoded response. Though perhaps we could smuggle those to the VA inside private fields of an error object.

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