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

Add respond with empty image #480

Merged
merged 5 commits into from
Dec 9, 2014
Merged

Add respond with empty image #480

merged 5 commits into from
Dec 9, 2014

Conversation

Telmo
Copy link
Contributor

@Telmo Telmo commented Nov 20, 2014

As discussed at RubyConf
Closes #86
All credit should go for @parolkar I just implemented his changes.

@repeatedly
Copy link
Member

Thanks!
I will check it later after back to Japan :)

@repeatedly repeatedly self-assigned this Nov 20, 2014
@@ -367,5 +376,5 @@ def send_response_nobody(code, header)
write data
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Removing this indentation is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably an editor issue on my side

@kiyoto
Copy link
Contributor

kiyoto commented Dec 4, 2014

@Telmo any update? I am thinking of creating a use case tutorial with this new feature.

@Telmo
Copy link
Contributor Author

Telmo commented Dec 4, 2014

I should be able to update the pul request early tomorrow morning

@kiyoto
Copy link
Contributor

kiyoto commented Dec 4, 2014

Awesome!

@Telmo
Copy link
Contributor Author

Telmo commented Dec 5, 2014

I am running into issues with the encoding and would appreciate some help. I have set the content type to

return ["200 OK", {'Content-type'=>'image/gif; charset=utf-8'}, EMPTY_GIF_IMAGE]

however it seems to always return as ASCII-8BIT no matter what I do.

You can see the error in TravisCI job #43097409

@kiyoto
Copy link
Contributor

kiyoto commented Dec 5, 2014

@Telmo

My preceding comment about 'false' => false has nothing to do with your encoding question. Sorry for the possible confusion.

About your encoding, it looks like Ruby's Net::HTTP is forcing ascii-8bit encoding when the response body is deemed to be GIF. To see this, I made the following change:

$ git diff lib/
diff --git a/lib/fluent/plugin/in_http.rb b/lib/fluent/plugin/in_http.rb
index 2321a22..6a44865 100644
--- a/lib/fluent/plugin/in_http.rb
+++ b/lib/fluent/plugin/in_http.rb
@@ -171,7 +171,7 @@ module Fluent
       end

       if @respond_with_empty_img
-        return ["200 OK", {'Content-type'=>'image/gif; charset=utf-8'}, EMPTY_GIF_IMAGE]
+        return ["200 OK", {'Content-type'=>'image/gif; charset=utf-8'}, 'hello world']
       else
         return ["200 OK", {'Content-type'=>'text/plain'}, ""]
       end

Then, when I manually run (from irb)

[36] pry(main)> res = Net::HTTP.post_form(uri, {"json" => '{"hello":100}'})
=> #<Net::HTTPOK 200 OK readbody=true>
[37] pry(main)> res.body.encoding
=> #<Encoding:UTF-8>

Note that the response's "content-type" was still "image/gif". So, I suggest that we update it as follows:

$ git diff lib
diff --git a/lib/fluent/plugin/in_http.rb b/lib/fluent/plugin/in_http.rb
index 2321a22..b221ef2 100644
--- a/lib/fluent/plugin/in_http.rb
+++ b/lib/fluent/plugin/in_http.rb
@@ -28,7 +28,7 @@ module Fluent
       super
     end

-    EMPTY_GIF_IMAGE = "GIF89a\u0001\u0000\u0001\u0000\x80\xFF\u0000\xFF\xFF\xFF\u0000\u0000\u0000,\u0000\u0000\u0000\u000
+    EMPTY_GIF_IMAGE = "GIF89a\u0001\u0000\u0001\u0000\x80\xFF\u0000\xFF\xFF\xFF\u0000\u0000\u0000,\u0000\u0000\u0000\u000

     config_param :port, :integer, :default => 9880
     config_param :bind, :string, :default => '0.0.0.0'

EDIT: minor clarifications.

@kiyoto
Copy link
Contributor

kiyoto commented Dec 5, 2014

@Telmo Apparently this is a long standing problem in Ruby. Here is the thread.

@repeatedly
Copy link
Member

It seems ruby level problem, right?
If the result of in_http is correct,
modify test with 'This is Ruby implementation issue.' like comment is enough for me.
Maybe, call encode?

@Telmo
Copy link
Contributor Author

Telmo commented Dec 8, 2014

I have tried calling encode in the string but the test fails with the same error.

@kiyoto
Copy link
Contributor

kiyoto commented Dec 9, 2014

@Telmo

With the following change to your PR, I managed to get it to pass the test. Can you check? (EDIT: I got rid of the unnecessary update to the post parameters.)

diff --git a/test/plugin/test_in_http.rb b/test/plugin/test_in_http.rb
index d2fdf7f..dd4cf5c 100644
--- a/test/plugin/test_in_http.rb
+++ b/test/plugin/test_in_http.rb
@@ -213,7 +213,8 @@ class HttpInputTest < Test::Unit::TestCase
       d.expected_emits.each {|tag,time,record|
         res = post("/#{tag}", {"json"=>record.to_json, "time"=>time.to_s})
         assert_equal "200", res.code
-        assert_equal Fluent::HttpInput::EMPTY_GIF_IMAGE, res.body
+        # Ruby returns ASCII-8 encoded string for GIF.
+        assert_equal Fluent::HttpInput::EMPTY_GIF_IMAGE, res.body.force_encoding("UTF-8")
       }
     end
   end

Updated test after hitting an issue with [Net:HTTP encoding](https://bugs.ruby-lang.org/issues/2567), thanks to @kioto for the help with this
@Telmo
Copy link
Contributor Author

Telmo commented Dec 9, 2014

@kiyoto thanks a lot for the help with the test. I have updated the PR, it implements the changes you made to the test and also fixes the issue with the false being a string instead of a boolean

@Telmo
Copy link
Contributor Author

Telmo commented Dec 9, 2014

the rubinious tests in Travis are taking forever. This test https://travis-ci.org/fluent/fluentd/jobs/43472778 has been going for 45 minutes at the moment of updating this comment and it is still installing the gemset

repeatedly added a commit that referenced this pull request Dec 9, 2014
Add respond with empty image
@repeatedly repeatedly merged commit 506b2f5 into fluent:master Dec 9, 2014
@repeatedly
Copy link
Member

Rubinius tests are not important so just merged. Thanks!

repeatedly added a commit that referenced this pull request Dec 10, 2014
Add respond with empty image
Conflicts:
	test/plugin/test_in_http.rb
@sonots sonots added the v0.10 label Dec 10, 2014
@sonots
Copy link
Member

sonots commented Dec 10, 2014

cherry-picked to v0.10 branch

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

Successfully merging this pull request may close these issues.

Add pixel tracking feature to in_http
4 participants