-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
nsqd: instrument aggregate message bytes on topics #1127
nsqd: instrument aggregate message bytes on topics #1127
Conversation
/cc @mreiferson @ploxiln |
82864c5
to
7bf1e79
Compare
I would use the word "bytes" instead of "size". There's also EDIT: eh probably better to do it in |
Yep. I have think about this also. But considering that in order not to put more burden to nsqd, i choose to use the existing HTTP header 'Content-Length' which is computed by distributed clients. But, we can not gather the TCP-based producer requests. |
We may have some messages with more than 1M bytes. This may trigger a serious CPU burden. |
7bf1e79
to
8e59b26
Compare
I will make a perf test on different message size and qps. Will feed that back later. |
|
but if you're really worried about efficiency, you can add-up the lengths in in |
Good suggestion. |
I think this feature was a good idea - thanks |
23386c6
to
014aa91
Compare
@ploxiln PTAL. |
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.
LGTM but I'll wait for @mreiferson to have a quick look
Seems that the cpu usage does not increase along with message size. https://docs.google.com/spreadsheets/d/1WsThzaqXjQY4-_cP1KUJVrXR61XYD-eeJdONEeqm2U8/edit#gid=1042961327. |
LGTM |
one last tiny thing - the commit title is no longer accurate, this is no longer specific to the http interface |
@ploxiln It seems that @mreiferson has changed the PR title. PTAL. It should be ready to be merged. |
We usually don't use the "squash and merge" option on github, which is what gives the opportunity to change the commit title and description, and defaults to the PR title. We usually "create a merge commit" so the commit in the PR is completely un-changed when introduced to the master branch. But it's not a big deal IMHO I'll just do it :) |
actually "squash and merge" button is disabled because matt really doesn't like it |
let me clarify: the commit title is "add topic message size metric for http protocol" |
Wait 3 minutes. |
014aa91
to
06baea2
Compare
@ploxiln PTAL. The Ci should be green. |
This PR adds message size per topic for HTTP protocol. In order to avoid more CPU usage to compute the length for messages, this PR leverage the HTTP header
Content-Length
for message length. As for MPUB, this includes the meta bytes involved in encoding multiple messages.