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

in_forward: Add source_hostname_key parameter. fix #804 #807

Merged
merged 2 commits into from
Aug 10, 2016

Conversation

repeatedly
Copy link
Member

No description provided.

@@ -47,6 +47,8 @@ def initialize
config_param :chunk_size_limit, :size, default: nil
desc 'Skip an event if incoming event is invalid.'
config_param :skip_invalid_event, :bool, default: false
desc "The field name of the client's hostname."
Copy link
Member

Choose a reason for hiding this comment

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

IMO it's better to have a note for performance degradation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tagomoris
Copy link
Member

This feature will be supported by porting into v0.14 API and using inject plugin helper.

@tagomoris tagomoris added the pending To be done in the future label Aug 3, 2016
@repeatedly
Copy link
Member Author

@tagomoris How to insert client's hostname by inject plugin helper?

@repeatedly repeatedly force-pushed the add-source-host-key-to-forward branch from 295731c to 91af142 Compare August 9, 2016 23:41
@repeatedly
Copy link
Member Author

Rebased.

@repeatedly repeatedly force-pushed the add-source-host-key-to-forward branch from 91af142 to 98ca682 Compare August 10, 2016 07:53
@tagomoris
Copy link
Member

Oh, I misunderstood the problem.

@tagomoris tagomoris removed the pending To be done in the future label Aug 10, 2016
@tagomoris
Copy link
Member

IMO, parameter name should be source_hostname_key instead of source_host_key to show this feature injects hostname, not host address or others explicitly.

@repeatedly
Copy link
Member Author

It is derived from in_tcp so you also think in_tcp's parameter should be renamed?

@tagomoris
Copy link
Member

Adding newer name to in_tcp sounds good idea.
in_forward plugin is used much widely than in_tcp, so I think we should NOT follow the bad way of in_tcp.

@repeatedly repeatedly changed the title in_forward: Add source_host_key parameter. fix #804 in_forward: Add source_hostname_key parameter. fix #804 Aug 10, 2016
@repeatedly
Copy link
Member Author

Renamed.

events.each {|tag,time,record|
entries << [time, record]
}
send_data ['tag1', entries].to_msgpack
Copy link
Member

Choose a reason for hiding this comment

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

I cannot understand why this code works well... time should be an ext typed value, but there are no info about it.

Copy link
Member

Choose a reason for hiding this comment

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

Correct way to do it is using Fluent::MessagePackFactory.

Copy link
Member Author

Choose a reason for hiding this comment

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

EventTime is serialized to int via to_msgpack and this is why this test works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, using Fluent::MessagePackFactory is better. "packed forward protocol with source_hostname_key" case does it.

Copy link
Member

Choose a reason for hiding this comment

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

It looks incorrect behavior to simulate out_forward... but not so serious for this test.

@tagomoris
Copy link
Member

LGTM.

@repeatedly
Copy link
Member Author

Okay. After test passed, I will merge this patch.

@repeatedly repeatedly merged commit f9d1715 into master Aug 10, 2016
@repeatedly repeatedly deleted the add-source-host-key-to-forward branch August 10, 2016 10:34
@repeatedly repeatedly added v0.12 feature request *Deprecated Label* Use enhancement label in general v0.14 labels Aug 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request *Deprecated Label* Use enhancement label in general v0.12 v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants