nsqadmin/nsqd: optimize /stats #925

jinhailang · 2017-08-08T03:05:40Z

optimize: If "topic" was specified trims to only that topic, if "channel" was specified trims to only that channel.
optimize: For the Topic page we just need the count of clients, we don't need all their info. For the Channel page, we do need all the client info, but not for all Channels, just for the one.

Nsqd and nsqadmin are compatible with older versions.

ploxiln · 2017-08-09T07:02:25Z

nsqadmin/static/js/lib/ajax_setup.js

@@ -7,6 +7,6 @@ $.ajaxPrefilter(function(options) {
        'X-UserAgent': USER_AGENT,
        'Accept': 'application/vnd.nsq; version=1.0'
    });
-    options['timeout'] = 20 * 1000;
+    options['timeout'] = 120 * 1000;


I don't think we want to change this timeout. We never want to wait this long for something to load.

(I understand you need to increase it in order for it to work for your setup. I guess we'd hope to make the stats requests just not take so long somehow.)

ploxiln · 2017-08-09T07:06:52Z

nsqd/stats.go

+		}
+	}
+	n.RUnlock()
+	realTopic.RLock()


I think you have to handle the possibility that the topic does not exist.

ploxiln · 2017-08-09T07:11:42Z

nsqd/stats.go

+	for _, c := range realChannels {
+		c.RLock()
+		clients := make([]ClientStats, 0, len(c.clients))
+		for _, client := range c.clients {


Another optimization that is possible here: For the Topic page we just need the count of clients, we don't need all their info. For the Channel page, we do need all the client info, but not for all Channels, just for the one.

Hmm ... we'd need to add a new param, to omit the client connection details from the stats response, to keep compatibility with anything out there that depends on the full stats responses. Luckily, the /stats endpoint will ignore params it is not expecting, so new nsqadmin will still work with old nsqd, it'll just get more info than it needs.

(The topic and channel params to /stats have existed for quite a while, since nsqd 0.3.6)

ploxiln · 2017-08-09T07:16:51Z

There's a lot of duplication between GetStats() and GetTopicStats() - it would be nice if most of the implementation were shared, e.g. GetStats() could just call the new function with a blank topic to mean "all topics".

jinhailang · 2017-08-10T03:07:38Z

nsqd/nsqd_test.go

@@ -387,7 +387,7 @@ func TestReconfigure(t *testing.T) {
 	nsqd.triggerOptsNotification()
 	test.Equal(t, 2, len(nsqd.getOpts().NSQLookupdTCPAddresses))

-	time.Sleep(200 * time.Millisecond)
+	time.Sleep(300 * time.Millisecond)


Here may be error, I guess sleep time is too short.

…ts.length

jinhailang · 2017-08-10T17:28:10Z

I've completed. If there is time, then again to help me review the code, thank you!
@ploxiln @mreiferson

mreiferson

looking better!

mreiferson · 2017-08-11T00:25:27Z

nsqadmin/http.go

+				totalClients += int64(len(cs.Clients))
+			} else {
+				totalClients += int64(cs.ClientCount)
+			}


rather than baking this logic into the client each time, we should just always return client_count as an int, separately from clients (the array).

mreiferson · 2017-08-11T00:25:42Z

internal/clusterinfo/data.go

@@ -530,7 +530,7 @@ func (c *ClusterInfo) GetNSQDTopicProducers(topic string, nsqdHTTPAddrs []string
 //
 // if selectedTopic is empty, this will return stats for *all* topic/channels
 // and the ChannelStats dict will be keyed by topic + ':' + channel
-func (c *ClusterInfo) GetNSQDStats(producers Producers, selectedTopic string) ([]*TopicStats, map[string]*ChannelStats, error) {
+func (c *ClusterInfo) GetNSQDStats(producers Producers, selectedTopic, selectChannel string) ([]*TopicStats, map[string]*ChannelStats, error) {


s/selectChannel/selectedChannel/

mreiferson · 2017-08-11T00:28:42Z

nsqd/http.go

+	var (
+		state TopicStats
+		stats []TopicStats
+	)


minor: we don't typically declare variables like this, we instead prefer var for each one.

mreiferson · 2017-08-11T00:30:39Z

nsqd/http.go

-	stats := s.ctx.nsqd.GetStats()
+	var (
+		state TopicStats
+		stats []TopicStats


the state variable is awkwardly named, how about we call these stats TopicStats and allStats []TopicStats?

mreiferson · 2017-08-11T00:31:09Z

nsqd/http.go

+	} else {
+		state, err = s.ctx.nsqd.GetChannelStats(topicName, channelName)
+	}
+	if nil != err {


minor: we typically prefer the variable in question to be on the left side of the comparison, e.g. if err != nil {

mreiferson · 2017-08-11T00:32:05Z

nsqd/http.go

+	if nil != err {
+		s.ctx.nsqd.logf(LOG_ERROR, "failed to get stats - %s", err)
+		return nil, http_api.Err{404, "topic/channel is not found"}
+	} else if nil == stats {


This doesn't need to be an else if, because the block above returns. Also, same minor note applies here, please move the variable to the left side of the comparison.

mreiferson · 2017-08-11T00:33:04Z

nsqd/http.go

+	} else if nil == stats {
+		stats = []TopicStats{state}
+	}
+
 	health := s.ctx.nsqd.GetHealth()
 	startTime := s.ctx.nsqd.GetStartTime()
 	uptime := time.Since(startTime)


After these changes above, Is any of the topic/channel looping code necessary below this line?

mreiferson · 2017-08-11T00:33:28Z

nsqd/stats.go

+	n.RLock()
+	var realTopic *Topic
+	for _, t := range n.topicMap {
+		if topic == t.name {


if t.name == topic {

same applies to function below

mreiferson · 2017-08-11T00:33:39Z

nsqd/stats.go

+	}
+	n.RUnlock()
+
+	if nil == realTopic {


if realTopic == nil {

same applies to function below

mreiferson · 2017-08-11T00:34:02Z

nsqd/stats.go

+
+	if nil == realTopic {
+		return TopicStats{}, errors.New("the topic is not found.")
+	} else {


since the above block returns, we can drop the else and un-indent this block

same applies to function below

andyxning · 2017-08-17T02:01:15Z

@mreiferson @jinhailang @ploxiln I have not found this PR ahead. But, i also proposed a PR #929 to make stats query more efficient and some of the thoughts are the same with @jinhailang

mreiferson · 2017-08-26T21:24:30Z

@jinhailang thanks for this, but going to close it in favor of #929!

jinhailang · 2017-08-27T02:57:10Z

I'm really sorry that I plan to revise this weekend. I have modified it, but can not commit it, if necessary, it may be available as an alternative.
I have been on the production environment to run for some time, the effect is very good, thank you for your help! @ploxiln @mreiferson

ploxiln · 2017-08-29T05:28:04Z

Just by the way, #929 does not include the option to exclude all individual client stats from topic stats, which I think is what gave the big performance gain for you. Someone could still possibly do that, separately after #929 is merged.

EDIT: missed the word not (it was sort of important)

andyxning · 2017-08-29T05:34:52Z

Someone could still possibly do that, separately after #929 is merged.

Agreed with @ploxiln .

@jinhailang The object is to make stats api for efficient. :)

mreiferson · 2017-08-29T17:09:02Z

Ahh, I missed that, good catch. Yes, we should do that too but let's do it as a follow up at this point.

optimize: gets stats

76374b5

jinhailang mentioned this pull request Aug 8, 2017

nsqd: /stats is slow for large deployments #920

Closed

mreiferson changed the title ~~optimize: gets stats~~ nsqadmin/nsqd: optimize /stats Aug 8, 2017

mreiferson added the perf label Aug 8, 2017

ploxiln reviewed Aug 9, 2017

View reviewed changes

nsqd/stats.go Outdated

}

}

n.RUnlock()

realTopic.RLock()

Copy link

Member

ploxiln Aug 9, 2017

Choose a reason for hiding this comment

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

I think you have to handle the possibility that the topic does not exist.

ploxiln reviewed Aug 9, 2017

View reviewed changes

jinhailang added 3 commits August 9, 2017 20:47

specify topic or channel

a288990

add ClientCount

bef0888

fix: nsqd_test may be error, when go=1.7.x,GOARCH=386

dae5477

jinhailang commented Aug 10, 2017

View reviewed changes

nsqadmin fix: when get topic stats, Use client_count instead of clien…

71ec515

…ts.length

mreiferson requested changes Aug 11, 2017

View reviewed changes

mreiferson mentioned this pull request Aug 26, 2017

nsqadmin/nsqd: refactor stats querying #929

Merged

mreiferson closed this Aug 26, 2017

jinhailang deleted the feature/stats branch September 22, 2017 10:47

mreiferson mentioned this pull request Jan 4, 2019

nsqd/nsqadmin: optimize retrieval of client stats #1118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsqadmin/nsqd: optimize /stats #925

nsqadmin/nsqd: optimize /stats #925

jinhailang commented Aug 8, 2017 •

edited

Loading

ploxiln Aug 9, 2017

ploxiln Aug 9, 2017

ploxiln Aug 9, 2017

ploxiln Aug 9, 2017

ploxiln commented Aug 9, 2017

jinhailang Aug 10, 2017

jinhailang commented Aug 10, 2017

mreiferson left a comment

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

mreiferson Aug 11, 2017

andyxning commented Aug 17, 2017

mreiferson commented Aug 26, 2017

jinhailang commented Aug 27, 2017

ploxiln commented Aug 29, 2017 •

edited

Loading

andyxning commented Aug 29, 2017 •

edited

Loading

mreiferson commented Aug 29, 2017

nsqadmin/nsqd: optimize /stats #925

nsqadmin/nsqd: optimize /stats #925

Conversation

jinhailang commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln commented Aug 9, 2017

Choose a reason for hiding this comment

jinhailang commented Aug 10, 2017

mreiferson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyxning commented Aug 17, 2017

mreiferson commented Aug 26, 2017

jinhailang commented Aug 27, 2017

ploxiln commented Aug 29, 2017 • edited Loading

andyxning commented Aug 29, 2017 • edited Loading

mreiferson commented Aug 29, 2017

jinhailang commented Aug 8, 2017 •

edited

Loading

ploxiln commented Aug 29, 2017 •

edited

Loading

andyxning commented Aug 29, 2017 •

edited

Loading