Skip to content

Regex capture groups silently require whitespace #397

Open

Description

@ohmann

Capture groups within the regex(es) used to parse log lines silently require whitespace to precede them. This is undocumented and is only detectable when using the --debugParse flag. In other words, it is likely to confuse users.

Example command:
synoptic.sh -r "(?<ITIME>),(?<ip>),(?<TYPE>)" [... other args ...]

Example debug parse snippet:

INFO: input: (?<ITIME>),(?<ip>),(?<TYPE>) 
INFO: processed: (?:\s*(?<ITIME>\S+)\s*),(?:\s+(?<ip>\S+)\s*),(?:\s+(?<TYPE>\S+)\s*) 
INFO: standard: (?:\s*(\S+)\s*),(?:\s+(\S+)\s*),(?:\s+(\S+)\s*)

Note that the capture group (?<ip>) is transformed into (?:\s+(?<ip>\S+)\s*), i.e., the capture group won't match a log line unless it is preceded by 1+ whitespace characters. This is somewhat deceptive, since the regex the user passed does not reference whitespace at all. The user might think that a line like 123,4.4.4.4,event would be matched, but in fact it will not be.

This only affects default behavior. Manually specifying capture group formatting, e.g., (?<ip>\S+), works as expected and does not silently require whitespace before it.

Activity

  1. bestchai commented on Sep 17, 2015

    @bestchai
    Member

    This is an important usability bug to fix. However, note that you'll have to fix existing args.txt files in the repository, as well. I think this is worthwhile, but the hardest part is to make sure that everything continues to work as before, at least for the examples we distribute. It might also be prudent to document this change on the main synoptic homepage, or in some form of release notes?

  2. ohmann commented on Sep 17, 2015

    @ohmann
    ContributorAuthor

    One option I considered is to deprecate the current -r argument (and state
    so) and to add a new argument (e.g., -R) that becomes the preferred one and
    does not mess with whitespace. This might seem messy, however, so updating
    everything is perhaps the better option. Adding to the usage message a
    warning that the behavior has changed might be useful if any current
    Synoptic users currently rely on the old behavior, sometimes update
    Synoptic, but don't regularly check the online documentation.

    On Thu, Sep 17, 2015 at 4:41 PM Ivan Beschastnikh notifications@github.com
    wrote:

    This is an important usability bug to fix. However, note that you'll have
    to fix existing args.txt files in the repository, as well. I think this is
    worthwhile, but the hardest part is to make sure that everything continues
    to work as before, at least for the examples we distribute. It might also
    be prudent to document this change on the main synoptic homepage, or in
    some form of release notes?

    —
    Reply to this email directly or view it on GitHub
    #397 (comment)
    .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions