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

nsqd: instrument aggregate message bytes on topics #1127

Conversation

andyxning
Copy link
Member

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.

@andyxning
Copy link
Member Author

/cc @mreiferson @ploxiln

@andyxning andyxning force-pushed the add_topic_message_size_metric_for_http_protocol branch from 82864c5 to 7bf1e79 Compare January 18, 2019 03:22
@ploxiln
Copy link
Member

ploxiln commented Jan 18, 2019

I would use the word "bytes" instead of "size".

There's also PUB() and MPUB() in nsqd/protocol_v2.go which is called for publishing by tcp-protocol clients. But I think the best place to add to the total is in Topic.put() and I think it's OK to do len(m.Body) to get the size of the message.

EDIT: eh probably better to do it in Topic.PutMessage() and Topic.PutMessages() similar to how messageCount is handled

@andyxning
Copy link
Member Author

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.

@andyxning
Copy link
Member Author

I think it's OK to do len(m.Body) to get the size of the message.

We may have some messages with more than 1M bytes. This may trigger a serious CPU burden.

@andyxning andyxning force-pushed the add_topic_message_size_metric_for_http_protocol branch from 7bf1e79 to 8e59b26 Compare January 18, 2019 04:24
@andyxning
Copy link
Member Author

I will make a perf test on different message size and qps. Will feed that back later.

@ploxiln
Copy link
Member

ploxiln commented Jan 18, 2019

len(m.Body) is not like libc strlen(), Body is a slice which stores its length explicitly

@ploxiln
Copy link
Member

ploxiln commented Jan 18, 2019

but if you're really worried about efficiency, you can add-up the lengths in in PutMessages() and only do a single atomic add :)

@andyxning
Copy link
Member Author

@ploxiln

but if you're really worried about efficiency, you can add-up the lengths in in PutMessages() and only do a single atomic add :)

Good suggestion.

@ploxiln
Copy link
Member

ploxiln commented Jan 18, 2019

I think this feature was a good idea - thanks

@andyxning andyxning force-pushed the add_topic_message_size_metric_for_http_protocol branch 2 times, most recently from 23386c6 to 014aa91 Compare January 18, 2019 04:58
@andyxning
Copy link
Member Author

@ploxiln PTAL.

Copy link
Member

@ploxiln ploxiln left a 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

@andyxning
Copy link
Member Author

Seems that the cpu usage does not increase along with message size. https://docs.google.com/spreadsheets/d/1WsThzaqXjQY4-_cP1KUJVrXR61XYD-eeJdONEeqm2U8/edit#gid=1042961327.

@mreiferson mreiferson changed the title add topic message size metric for http protocol nsqd: instrument aggregate message bytes on topics Jan 18, 2019
@mreiferson
Copy link
Member

LGTM

@ploxiln
Copy link
Member

ploxiln commented Jan 18, 2019

one last tiny thing - the commit title is no longer accurate, this is no longer specific to the http interface

@andyxning
Copy link
Member Author

@ploxiln It seems that @mreiferson has changed the PR title. PTAL. It should be ready to be merged.

@ploxiln
Copy link
Member

ploxiln commented Jan 20, 2019

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 :)

@ploxiln
Copy link
Member

ploxiln commented Jan 20, 2019

actually "squash and merge" button is disabled because matt really doesn't like it

@ploxiln
Copy link
Member

ploxiln commented Jan 20, 2019

let me clarify: the commit title is "add topic message size metric for http protocol"

@andyxning
Copy link
Member Author

let me clarify: the commit title is "add topic message size metric for http protocol"

Wait 3 minutes.

@andyxning andyxning force-pushed the add_topic_message_size_metric_for_http_protocol branch from 014aa91 to 06baea2 Compare January 20, 2019 03:27
@andyxning
Copy link
Member Author

@ploxiln PTAL. The Ci should be green.

@ploxiln ploxiln merged commit 38fcacc into nsqio:master Jan 20, 2019
@andyxning andyxning deleted the add_topic_message_size_metric_for_http_protocol branch January 20, 2019 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants