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

Implement mackerel-plugin-twemproxy #283

Merged
merged 2 commits into from Nov 22, 2016

Conversation

yoheimuta
Copy link
Contributor

We plan to use this plugin in production. After our test trial in production, we are sure that it's helpful for operators monitoring twemproxy(s).

In my opinion, twemproxy is a somewhat defacto standard proxy for redis and memcached, so I'm posting this PR.

@yoheimuta
Copy link
Contributor Author

ping. (because it passed 13 days)

@stefafafan
Copy link
Contributor

Thanks for the pull-request and sorry for the late reply.
We will take a look at it in a moment so please wait a bit. 🙇

@yoheimuta
Copy link
Contributor Author

Thanks for the reply 😌

@stefafafan
Copy link
Contributor

I took a look at the code and noticed you decided to skip fragments and server_ejected_at . Is there a reason you made that decision?

@yoheimuta
Copy link
Contributor Author

@stefafafan

I took a look at the code and noticed you decided to skip fragments and server_ejected_at

Yes, I skip fragments and server_ejected_at.

fragments "# fragments created from a multi-vector request"
https://github.com/twitter/twemproxy#observability

timestamp when server was ejected in usec since epoch
https://github.com/twitter/twemproxy/blob/0f9e1baf06db478065ba43402b975c57a567d00f/src/nc_stats.h#L40

Is there a reason you made that decision?

Yes.

fragments

We don't mind any number of fragments created, because we think fragments is relatively not important for understanding the twemproxy loads in our usage.

server_ejected_at

I couldn't image the graph to point those timestamp data(= タイムスタンプ値をグラフで表現するわかりやすい方法が思い浮かばなかったので、今回の実装から除外しました).

Summarizing the above, I decided to skip these parameters because other users should just implement to add these when users needed appear.
( I should write this note in pr description 🙇 )

@stefafafan
Copy link
Contributor

@yoheimuta
Thanks for the description.

I decided to skip these parameters because other users should just implement to add these when users needed appear

I see, that makes sense. Perhaps write a note about fragments and server_ejected_at in the README.md might be great if you don't mind.

Other than that, great work!

@yoheimuta
Copy link
Contributor Author

@stefafafan Thanks for the review.

I wrote a note in the README.md 11beca9.

@stefafafan
Copy link
Contributor

@yoheimuta
Sorry to make you wait, thanks for adding to the readme!

@stefafafan stefafafan merged commit 3a68cf4 into mackerelio:master Nov 22, 2016
@yoheimuta yoheimuta deleted the implement-plugin-twemproxy branch November 22, 2016 02:01
@yoheimuta
Copy link
Contributor Author

Thanks!

@stanaka stanaka mentioned this pull request Nov 29, 2016
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.

None yet

2 participants