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

Added the processing when did not match the rules. #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

studio3104
Copy link

Hi.
I've added the action to take if do not match the rules.

Changes
・Do not emit if the tag is not replaced.
・Output an error message depending on the situation.
・Set the rules required the 'key' and 'pattern'.

Thank you confirmation.

@@ -21,10 +22,14 @@ def configure(conf)
@rules = conf.elements.select { |element|
element.name == 'rule'
}.each { |element|
if element.has_key?("pattern")
if element["pattern"] && element["key"]
Copy link
Owner

Choose a reason for hiding this comment

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

ここ、なんでelement["pattern"]に変更したんでしょうか。

element["pattern"]だと、ルールにpatternと書いただけで空文字が入ってしまう(つまりtrueになる)ので、patternというキーを設定するってことは、それが空文字だろうがなんだろうが、なんか設定する意思を表示したものとして扱うためにhas_key?したような記憶があります。

でもあんま憶えてないので嘘かもですが…

@studio3104
Copy link
Author

ご指摘の箇所を修正し、エラーメッセージのテストを追加しました。
インスタンス変数での実装以外が思いつかなかったので、基本的には前回のコミットと同じ処理です。。

ご確認よろしくお願いいたします。

@@ -52,7 +63,14 @@ def emit(tag, es, chain)

es.each do |time, record|
filtered_tag, record = rewrite(tag, record)
Engine.emit(filtered_tag, time, record) if filtered_tag && record
if _tag == filtered_tag
if !@warn_msg
Copy link
Owner

Choose a reason for hiding this comment

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

ここの条件、通ることあるんですかね?

Copy link
Author

Choose a reason for hiding this comment

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

test_emit d3のケースで通過します。
・replaceが設定されているが、同じkeyに対してのruleで、append_to_tagが設定されていない場合

コメント書いてて気づきましたがコレはapply_ruleのところに書くべきですね・・・

Copy link
Owner

Choose a reason for hiding this comment

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

hmhm じゃあそっちに移動しますかねー

Copy link
Author

Choose a reason for hiding this comment

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

あ、でもその場合はconfigErrorにしたほうが良い気もしてきました。
どう思われますか?

Copy link
Owner

Choose a reason for hiding this comment

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

Error投げるとfluentdとまりますよね? 止まったほうがいいのかなー

Copy link
Author

Choose a reason for hiding this comment

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

起動しなくなりますね。
でも、該当する場合は未来永劫emitしないruleなわけですからね・・・

Copy link
Owner

Choose a reason for hiding this comment

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

ああ、最初のconfigのところでErrorにするわけですね。それならいいとおもいます。

Copy link
Author

Choose a reason for hiding this comment

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

言葉足らずで申し訳ありません。。
では、cofigureメソッドでチェックして、ダメな場合はerrorにしますね。

Copy link
Owner

Choose a reason for hiding this comment

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

例外をテストするなら、assert_raise使う感じですねー
http://www.ruby-doc.org/stdlib-1.9.3/libdoc/test/unit/rdoc/Test/Unit/Assertions.html#method-i-assert_raise

@studio3104
Copy link
Author

・単一のkeyに対して最終的にappend_to_tagがセットされていない場合
・(add|remove)_prefixが設定されいない場合

に、raise ConfigErrorするようにしました。
醜いコードでコミット重ねてしまって大変恐縮ですが、ご確認よろしくお願いいたしますm(__)m

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.

2 participants