nsqadmin/nsqd: refactor stats querying #929

andyxning · 2017-08-16T14:30:40Z

As nsqd stats api supports format, topic and channel url parameters. When displaying info for a specific topic and channel, nsqadmin should query nsqd stats api with the specified topic and channel instead querying all channels under the specified topic and laterly filter out the specified channel stats.

Query stats on demand can reduce the time for nsqadmin api to return and will reduce the possibility that nsqadmin report timeout when to query a topic with many channels and each channel with many consumer clients.

/cc @mreiferson @ploxiln

mreiferson

Thanks for submitting this @andyxning, just a few minor comments.

Are there any other test additions we should make here?

mreiferson · 2017-08-26T21:19:20Z

internal/clusterinfo/data.go

-			endpoint := fmt.Sprintf("http://%s/stats?format=json", addr)
+
+			var endpoint string
+			if len(selectedChannel) != 0 {


This isn't entirely correct, as it doesn't test for selectedTopic, let's just adopt the code in #925 wholesale for this block:

if selectedTopic == "" { endpoint = fmt.Sprintf("http://%s/stats?format=json", addr) } else if selectChannel == "" { endpoint = fmt.Sprintf("http://%s/stats?topic=%s&format=json", addr, selectedTopic) } else { endpoint = fmt.Sprintf("http://%s/stats?topic=%s&channel=%s&format=json", addr, selectedTopic, selectChannel) }

Let me humbly offer my own variant here:

endpoint := fmt.Sprintf("http://%s/stats?format=json", addr) if selectedTopic != "" { endpoint += "&topic=" + selectedTopic if selectedChannel != "" { endpoint += "&channel=" + selectedChannel } }

Thanks for your review and the more simplified and clearer refactor. @ploxiln @mreiferson

mreiferson · 2017-08-26T21:20:21Z

nsqd/stats.go

-	for _, t := range n.topicMap {
-		realTopics = append(realTopics, t)
+	var realTopics []*Topic
+	if len(topic) == 0 {


Minor, but In Go, this test should just be:

if topic == "" {

mreiferson · 2017-08-26T21:21:20Z

nsqd/stats.go

+		}
+	} else {
+		realTopics = make([]*Topic, 0, 1)
+		realTopics = append(realTopics, n.topicMap[topic])


These two lines can be simplified to:

realTopics = []*Topic{n.topicMap[topic]}

mreiferson · 2017-08-26T21:21:55Z

nsqd/stats.go

-		for _, c := range t.channelMap {
-			realChannels = append(realChannels, c)
+		var realChannels []*Channel
+		if len(channel) == 0 {


Same idiomatic improvement here:

if channel == "" {

mreiferson · 2017-08-26T21:22:15Z

nsqd/stats.go

+			}
+		} else {
+			realChannels = make([]*Channel, 0, 1)
+			realChannels = append(realChannels, t.channelMap[channel])


And same simplification here:

realChannels = []*Channel{t.channelMap[channel]}

andyxning · 2017-08-29T02:29:29Z

@mreiferson @ploxiln Comments addressed. PTAL.

ploxiln · 2017-08-29T03:05:08Z

nsqd/stats_test.go

@@ -32,7 +32,7 @@ func TestStats(t *testing.T) {
 	identify(t, conn, nil, frameTypeResponse)
 	sub(t, conn, topicName, "ch")

-	stats := nsqd.GetStats()
+	stats := nsqd.GetStats(topicName, "ch")


it's probably a good idea to expand this test a bit - put a message to one other topic and channel

Sounds reasonable. Done.

ploxiln · 2017-08-29T03:05:50Z

looking pretty good to me

andyxning · 2017-08-29T04:03:48Z

@ploxiln Comments addressed. PTAL.

mreiferson · 2017-08-29T17:06:56Z

nsqd/stats.go

+			realTopics = append(realTopics, t)
+		}
+	} else {
+		realTopics = []*Topic{n.topicMap[topic]}


Just realized we're not checking if the topic exists in topicMap, can you add that check here?

@mreiferson Good catch! I also have enhanced the test for both of none exist topic and channel.

PTAL.

mreiferson · 2017-08-29T17:07:14Z

nsqd/stats.go

+				realChannels = append(realChannels, c)
+			}
+		} else {
+			realChannels = []*Channel{t.channelMap[channel]}


same here, need to check if channel is in channelMap first

mreiferson · 2017-09-01T15:47:59Z

@andyxning fixed up the commits, LGTM, thanks!

andyxning force-pushed the refactor_stats_qeury_for_nsqadmin branch from 1c4dc28 to 8a4ba22 Compare August 16, 2017 14:44

andyxning mentioned this pull request Aug 17, 2017

nsqadmin/nsqd: optimize /stats #925

Closed

andyxning force-pushed the refactor_stats_qeury_for_nsqadmin branch 3 times, most recently from 50027f4 to d0ae961 Compare August 17, 2017 15:05

mreiferson changed the title ~~refactor stats query for nsqadmin~~ nsqadmin/nsqd: refactor stats querying Aug 26, 2017

mreiferson added the perf label Aug 26, 2017

mreiferson requested changes Aug 26, 2017

View reviewed changes

andyxning force-pushed the refactor_stats_qeury_for_nsqadmin branch from d0ae961 to 1ab3405 Compare August 29, 2017 02:29

ploxiln reviewed Aug 29, 2017

View reviewed changes

andyxning mentioned this pull request Aug 29, 2017

add client metrics nsqio/nsqio.github.io#26

Closed

andyxning force-pushed the refactor_stats_qeury_for_nsqadmin branch from 1ab3405 to bb8fc4c Compare August 29, 2017 03:56

mreiferson requested changes Aug 29, 2017

View reviewed changes

andyxning force-pushed the refactor_stats_qeury_for_nsqadmin branch 3 times, most recently from 1bd8e4a to bf589db Compare August 30, 2017 13:49

andyxning added 4 commits September 1, 2017 08:47

internal/clusterinfo: when topic/channel specified, make targeted query

b06c3d9

nsqd: add topic/channel to GetStats API

18f5c86

nsq_stat: update GetNSQDStats API

d0f20d1

nsqadmin: update GetNSQDStats API

9fe3205

mreiferson force-pushed the refactor_stats_qeury_for_nsqadmin branch from bf589db to 9fe3205 Compare September 1, 2017 15:47

mreiferson approved these changes Sep 1, 2017

View reviewed changes

mreiferson merged commit 4f80051 into nsqio:master Sep 1, 2017

andyxning deleted the refactor_stats_qeury_for_nsqadmin branch September 1, 2017 22:28

jehiah mentioned this pull request Jan 8, 2018

nsqadmin: error querying nsqd stats for #ephemeral channel #982

Merged

ploxiln mentioned this pull request Jul 31, 2018

nsqd: /stats is slow for large deployments #920

Closed

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: refactor stats querying #929

nsqadmin/nsqd: refactor stats querying #929

andyxning commented Aug 16, 2017 •

edited

Loading

mreiferson left a comment

mreiferson Aug 26, 2017

ploxiln Aug 26, 2017

mreiferson Aug 26, 2017

andyxning Aug 29, 2017 •

edited

Loading

mreiferson Aug 26, 2017

andyxning Aug 29, 2017

mreiferson Aug 26, 2017

andyxning Aug 29, 2017

mreiferson Aug 26, 2017

andyxning Aug 29, 2017

mreiferson Aug 26, 2017

andyxning Aug 29, 2017

andyxning commented Aug 29, 2017

ploxiln Aug 29, 2017

andyxning Aug 29, 2017

ploxiln commented Aug 29, 2017

andyxning commented Aug 29, 2017

mreiferson Aug 29, 2017

andyxning Aug 30, 2017

mreiferson Aug 29, 2017

mreiferson commented Sep 1, 2017

nsqadmin/nsqd: refactor stats querying #929

nsqadmin/nsqd: refactor stats querying #929

Conversation

andyxning commented Aug 16, 2017 • edited Loading

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

andyxning Aug 29, 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

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 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln commented Aug 29, 2017

andyxning commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreiferson commented Sep 1, 2017

andyxning commented Aug 16, 2017 •

edited

Loading

andyxning Aug 29, 2017 •

edited

Loading