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

Add alternate mechanism for parsing log lines #494

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

msakrejda
Copy link
Contributor

@msakrejda msakrejda commented Dec 27, 2023

Our current log line parsing mechanism relies on matching each log
line to one of a set of recognized prefix patterns, then pulling
relevant information out of the prefix "manually" according to their
regex groups.

This works, but is difficult to maintain and requires customers to use
one of our prefixes. Occasionally, we add prefixes (e.g., to support a
provider that does not allow changing log_line_prefix), but this is
error-prone and requires the customer to wait for a collector release.

This patch takes a different approach: we inspect the log_line_prefix,
generate a regular expression to match lines coming with that specific
prefix pattern, and use that to pull relevant metadata out of the log
stream.

Currently, this patch is a proof of concept, with only
ParseLogLineWithPrefix implemented. However, it passes all the tests
of our existing ParseLogLineWithPrefix function. Given it uses a
single purpose-built regex, it can also be more lenient with patterns
for specific fields in a match (e.g., roles with spaces like "Admin
User").

TODO:

  • Get server's log_line_prefix and store where it can be passed to
    ParseLogLineWithPrefix
  • Avoid re-compiling the prefix regex on every call to
    ParseLogLineWithPrefix
  • Add handling for syslog log line format
  • Support calling both old and new ParseLogLineWithPrefix via
    collector setting to have an escape hatch in case of issues
    • we could do this via a db_log_line_prefix setting or similar, and
      set that to 'auto' (the new behavior), 'legacy' (the current behavior)
      or a literal prefix (the new behavior but prefix can't be read from
      settings for whatever reason)
  • Fix naming (right now, the core method is just called
    ParseLogLineWithPrefix2, but that's not ideal)
  • Add tests for untested prefixes (some of the existing
    allegedly-supported prefixes do not have tests in the old code,
    so it's not clear if the new code supports them—they should just
    work, but it'd be good to verify this.)
  • Warn when prefix does not include database or role
  • Benchmark (this is almost certainly faster, but it'd be good to
    confirm)

I'm not sure about the best path forward for these. I think we could
store log_line_prefix along with the LogTimezone we are already
storing on state.Server. If we follow the same pattern, it would be
easy to refresh it on every full snapshot. We may want to keep the
regex on state.Server as well, so we can recompile it as necessary (if
the prefix changes) but otherwise leave it alone. We'd need to keep
the matchers on the state.Server object as well.

Support for calling both the old and new code may be unnecessary, but
since this is a rewrite of a fundamental piece of collector code, it's
probably wise.

I'd love to hear any thoughts or concerns.

@msakrejda
Copy link
Contributor Author

Oh, another thing not covered is parsing lines received through syslog. I don't understand how syslog interacts with log_line_prefix--do we only support a single prefix when syslog is used?

@msakrejda
Copy link
Contributor Author

I should also add the case from #449; I think this fixes it.

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

1 participant