-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Migrate in_monitor_agent to v0.14 API #1151
Migrate in_monitor_agent to v0.14 API #1151
Conversation
More test cases are very good for this change. |
|
Leave it as-is. It's far different from normal output plugins. |
I've tried to add test cases for in_monitor_agent. |
matches.each { |rule| | ||
if rule.match?(tag) | ||
if rule.collector.is_a?(Output) | ||
if rule.collector.is_a?(Fluent::Output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fluent::Plugin::Output
looks correct instead of Fluent::Output
(this means v0.12 plugin instances only).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I've missed it. 😰
I'll fix it.
Previous assertions are too strict to test.
I've tried to reduce too strict assertions and add tests. |
@@ -330,7 +282,7 @@ def all_plugins | |||
# from the plugin `pe` recursively | |||
def self.collect_children(pe, array=[]) | |||
array << pe | |||
if pe.is_a?(MultiOutput) && pe.respond_to?(:outputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required to collect data for old MultiOutput plugins.
Old (v0.12) MultiOutput plugin are not children of Fluent::Plugin::MultiOutput
, but Fluent::Compat::MultiOutput
(and it is child of Fluent::Plugin::BareOutput
).
fluent-plugin-forest and other plugins are affected with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see....
I've added additional commit to resolve your commented problem: 03cf62e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should do pe.is_a?(Fluent::Plugin::MultiOutput) || pe.is_a?(Fluent::MultiOutput)
(both respond to #outputs
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cosmo0920 ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pong. I've pushed a commit to fix this.
ad54714
to
99370c5
Compare
99370c5
to
3a0267b
Compare
matches.each { |rule| | ||
if rule.match?(tag) | ||
if rule.collector.is_a?(Output) | ||
if rule.collector.is_a?(Fluent::Plugin::Output) || rule.collector.is_a?(Fluent::Output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should both Fluent::Plugin::Output
and Fluent::Output
as output plugins, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
LGTM. |
Thanks! 😉 |
I've migrated in_monitor_agent plugin to v0.14 API.
Remaining tasks
Not for this PR tasks