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

HTTP リクエストヘッダを指定できる様にするためにオプションパラメータを受け付ける #1

Merged
merged 3 commits into from
Jan 30, 2018

Conversation

koshigoe
Copy link
Contributor

Bugsnag::Delivery::{DeliverClass}#deliver の第四引数 options (Hash)を使って HTTP リクエストヘッダを指定する実装がある。

https://github.com/bugsnag/bugsnag-ruby/blob/44c6ab14d7b82f6ed39109225897e7e49927085c/lib/bugsnag/delivery/synchronous.rb#L43-L44

https://github.com/bugsnag/bugsnag-ruby/blob/44c6ab14d7b82f6ed39109225897e7e49927085c/lib/bugsnag.rb#L117-L119

bugsnag-delivery-fluent でも同様に HTTP リクエストヘッダを options 引数経由で受け渡しできる様にする前提で、このプラグインを拡張する。

@@ -0,0 +1,3 @@
machine:
ruby:
version: 2.1.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gem::InstallError: public_suffix requires Ruby version >= 2.1.
Gem::InstallError: strptime requires Ruby version ~> 2.0.

https://circleci.com/gh/feedforce/fluent-plugin-bugsnag/7

Choose a reason for hiding this comment

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

2.1 はもう EOL を迎えたので、2.2 以上で良いような。
https://www.ruby-lang.org/ja/downloads/

gem の CI は複数バージョンの Ruby の指定が簡単な https://travis-ci.org/ がお手軽なイメージです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

travis

わかります。雛形で作った(と思わしき)設定ファイルが残っているところとかも気持ち悪さが若干あったりします。

2.1 はもう EOL を迎えたので、2.2 以上で良いような。

CI を通す以上の理由はなかったです。
td-agent 同梱の Ruby のバージョンも把握できておらず、今回は CI 通すだけのバージョン指定とさせてもらいたいです。

@@ -17,13 +17,14 @@ def format(tag, time, record)

def write(chunk)
chunk.msgpack_each do |(tag,time,record)|
request(record['url'], record['body'])
options = record['options'] || {}
request(record['url'], record['body'], options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

```
Gem::InstallError: public_suffix requires Ruby version >= 2.1.
Gem::InstallError: strptime requires Ruby version ~> 2.0.
```
@koshigoe koshigoe force-pushed the feature/allow-to-set-request-headers branch from fc36246 to 4c4c35f Compare January 30, 2018 08:17
@masutaka
Copy link

https://github.com/bugsnag/bugsnag-ruby/blob/44c6ab14d7b82f6ed39109225897e7e49927085c/lib/bugsnag/delivery/synchronous.rb#L43-L44

あまり把握はできていないのですが、 bugsnag/bugsnag-ruby#411 で Session tracking という機能が実装され、それ関連で必要になったのですかね。

@@ -0,0 +1,3 @@
machine:
ruby:
version: 2.1.0

Choose a reason for hiding this comment

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

2.1 はもう EOL を迎えたので、2.2 以上で良いような。
https://www.ruby-lang.org/ja/downloads/

gem の CI は複数バージョンの Ruby の指定が簡単な https://travis-ci.org/ がお手軽なイメージです。

@@ -36,13 +37,22 @@ def request(url, body)
http.verify_mode = OpenSSL::SSL::VERIFY_NONE
end

request = Net::HTTP::Post.new(path(uri), {"Content-Type" => "application/json"})
headers = options.key?('headers') ? options['headers'] : {}

Choose a reason for hiding this comment

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

📝 option の中から 'headers' だけ頂く。それ以外は捨てる。

@masutaka
Copy link

@koshigoe あまり重要ではありませんが、 #1 (comment) だけ気になりました。CI を通す程度で良ければ、そのままでもかまわないです。

@koshigoe
Copy link
Contributor Author

あまり把握はできていないのですが、 bugsnag/bugsnag-ruby#411 で Session tracking という機能が実装され、それ関連で必要になったのですかね。

わたしも詳細把握してませんが、そんな感じですね。
Bugsnag API の(ペイロードの)バージョンを上げる必要があり、そのバージョンではリクエストヘッダの使用が推奨されているから合わせた、的な。

@koshigoe
Copy link
Contributor Author

この PR はこの状態でマージしてしまいます。

@koshigoe koshigoe merged commit 84c2dee into master Jan 30, 2018
@koshigoe koshigoe deleted the feature/allow-to-set-request-headers branch January 30, 2018 09:29
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