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

inverted: true should always do the opposite of inverted: false #96

Open
ferbar opened this issue Apr 1, 2020 · 7 comments
Open

inverted: true should always do the opposite of inverted: false #96

ferbar opened this issue Apr 1, 2020 · 7 comments

Comments

@ferbar
Copy link

ferbar commented Apr 1, 2020

Following configuration:

<match a.**>
  @type rewrite_tag_filter
# fist rule
  <rule>
    key might_be_empty
    pattern ^(.*)$
    tag $1.${tag}
  </rule>
#2nd rule
  <rule>
    key might_be_empty
    pattern ^(.*)$
    invert true
    tag unknown.${tag}
  </rule>
</match>

If we test the rewrite filter with two log messages:

  1. The key "might_be_empty" contains "string"
    => pattern matches
    => new tag is string.something, perfect
  2. The key "might_be_empty" is empty
    first rule:
    => "check if recordvalue.isempty returns true in https://github.com/fluent/fluent-plugin-rewrite-tag-filter/blob/master/lib/fluent/plugin/out_rewrite_tag_filter.rb#L102
    => next
    2nd rule:
    match_operator != MATCH_OPERATOR_EXCLUDE fails so, no next this time
    regexp_last_match(regexp, rewritevalue) returns true because /.*/ matches on an empty string
    => next if last_match because last_match is true
    => log message discarded with the message "rewrite_tag_filter: tag has not been rewritten"

Possible solutions would be

  1. no check if the rewritevalue is empty
  2. if rewritevalue is empty then always return true
  3. if "an empty rewritevalue is always a no-match" is a defined behavior it should be documented.
  4. refuse pattern which match on empty strings + invert:true

Please let me know your preferences, I will file a PR

A workaround is to write a regex that requires at least one character. For example /.+/

@okkez
Copy link
Contributor

okkez commented Apr 5, 2020

How do you think about this? @ganmacs @y-ken

See also #16 #61

@AlexOQ
Copy link

AlexOQ commented Oct 22, 2020

While having the parameter can be nice, I believe can be achieved with ^(.*$|$) pattern. Then second rule is the invert, and voila. This should catch all logs that have the field might_be_empty.

@y-ken
Copy link
Member

y-ken commented Mar 6, 2024

Hi @okkez ( CC: @daipom / @ashie )
Sorry for the late reply. I would like to restart the task to resolve this issue.
I think this issue needs to be discussed to resolve that records are lost if the value is nil or empty.
If my understanding is correct, I feel that just merging #16 is an straightforward solution.
If you have any concerns, please let us know.

@daipom
Copy link
Contributor

daipom commented Mar 6, 2024

@y-ken Thanks! Sorry I'm not aware of this.
I have not checked this in detail yet, but it certainly appears that the record can be discarded if the key's value is empty.
It would not be the intended behavior.

next if rewritevalue.empty? && match_operator != MATCH_OPERATOR_EXCLUDE

@daipom
Copy link
Contributor

daipom commented Mar 6, 2024

My understanding is as follows. Is it correct?

  • This is not a problem limited to the invert option.
  • The expected behavior of the config is as follows.
    • The first rule matches the empty value.
    • The second rule does not match the empty value.

pattern ^(.*)$ should match all values including the empty value.
The current behavior is wrong at this point.

So, if inverting pattern ^(.*)$, it should not match any value.

@daipom
Copy link
Contributor

daipom commented Mar 6, 2024

My understanding is as follows. Is it correct?

* This is not a problem limited to the `invert` option.

* The expected behavior of the config is as follows.
  
  * The first rule matches the empty value.
  * The second rule does not match the empty value.

pattern ^(.*)$ should match all values including the empty value. The current behavior is wrong at this point.

So, if inverting pattern ^(.*)$, it should not match any value.

Sorry, I missed one point.
In addition to whether the value is an empty string or not, we also need to consider whether the key exists or not.

When the key does not exist, is it expected that all patterns do not match?

Note: This changes checking nil to checking empty.

405c5e0

@y-ken
Copy link
Member

y-ken commented Mar 6, 2024

Thank you for prompt reply. Yeah, It's certainly not an inverted issue, so I'd like to continue discussion at #16.
I will reply above questions in the #16 .

Thank you.

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

No branches or pull requests

5 participants