nsqd: add producer client connections to /stats #881

sparklxb · 2017-04-10T07:49:40Z

add /producer_stats endpoint for nsqd. #879

ploxiln · 2017-04-10T19:29:59Z

I think the official client libraries either only produce or only consume on a single connection to nsqd, so if that requirement was formalized, it would remove the need to add another counter to the client struct ("messages" could be re-used).

It is completely valid, and not uncommon, to publish to multiple different topics from a single producer tcp client connection to nsqd. But often only a single topic will be published to by a process / connection. It would be a useful enhancement to associate a producer client tcp connection with the topic(s) it publishes to, so one can see producer stats for different topics differentiated. Perhaps the most practical way is to limit this association to one topic, and if more than one, then just consider it "multiple".

sparklxb · 2017-04-11T02:14:33Z

@ploxiln I did consider what you had mentioned. If producer client publishs more than one topic, there is more work to do, such as providing separate client struct for topics published by one producer or changing client struct (but make it more dirty or complicated). So just wait for owners' comment.

mreiferson · 2017-04-11T18:28:49Z

I think the official client libraries either only produce or only consume on a single connection to nsqd, so if that requirement was formalized, it would remove the need to add another counter to the client struct ("messages" could be re-used).

Yea, but it's not a real restriction so I don't think we can reasonably do that.

It is completely valid, and not uncommon, to publish to multiple different topics from a single producer tcp client connection to nsqd.

Feels like we can defer tracking stats at that granularity for this first pass.

mreiferson · 2017-04-11T18:30:02Z

nsqd/client_v2.go

@@ -346,6 +349,10 @@ func (c *clientV2) SendingMessage() {
 	atomic.AddUint64(&c.MessageCount, 1)
 }

+func (c *clientV2) PublishMessage(msgNum uint64) {


s/Publish/Published

mreiferson · 2017-04-11T18:30:45Z

nsqd/http.go

@@ -52,6 +52,7 @@ func newHTTPServer(ctx *context, tlsEnabled bool, tlsRequired bool) *httpServer
 	router.Handle("POST", "/pub", http_api.Decorate(s.doPUB, http_api.V1))
 	router.Handle("POST", "/mpub", http_api.Decorate(s.doMPUB, http_api.V1))
 	router.Handle("GET", "/stats", http_api.Decorate(s.doStats, log, http_api.V1))
+	router.Handle("GET", "/producer_stats", http_api.Decorate(s.doProducerStats, log, http_api.V1))


I'm still strongly in favor of keeping this all in /stats, perhaps with a flag to enable.

A flag to return one or all?

I think he means "all", e.g. a flag/param like "with_producers=true" or "with_producers=1" or "producers=1" (brevity to taste ;).

If there was (minimal) channel information associated with the producers, then this new flag could combine with the existing "topic" flag in the logical way.

mreiferson · 2017-04-11T18:31:51Z

nsqd/nsqd.go

@@ -51,7 +55,8 @@ type NSQD struct {
 	errValue  atomic.Value
 	startTime time.Time

-	topicMap map[string]*Topic
+	topicMap  map[string]*Topic
+	producers map[int64]Producer


Calling this variable producers seems misleading because we can't really know or categorize for sure. I'd rather call this clients.

mreiferson · 2017-04-11T18:33:24Z

nsqd/protocol_v2.go

@@ -796,6 +797,9 @@ func (p *protocolV2) PUB(client *clientV2, params [][]byte) ([]byte, error) {
 		return nil, protocol.NewFatalClientErr(err, "E_PUB_FAILED", "PUB failed "+err.Error())
 	}

+	p.ctx.nsqd.AddClient(client.ID, client)


why do we add the client every time here, can't we do this once when any client connects? This relates to my comments above about variable naming.

But we can't make sure if a client is a producer, until it's first PUB or MPUB.

I think what mreiferson is suggesting is to keep a set of all clients (added to the set when they connect). Then, when generating publisher statistics, you can iterate over all clients and include them only if they have published any messages.

ploxiln · 2017-04-11T18:33:47Z

nsqd/nsqd.go

+// AddClient adds a client to the producers map
+func (n *NSQD) AddClient(clientID int64, client Producer) {
+	n.Lock()
+	defer n.Unlock()


if we're really going to have to call AddClient() on every single PUB, we can make it a bit more efficient by not using defer

... but if there was another client property, from which it could be determined if the client connection had already published anything, AddClient() could usually be skipped

EDIT: possibly obsoleted by comments above

I think we can check if PublishCount property I added is greater than 0.

ploxiln · 2017-04-11T18:38:52Z

Just to be clear, @mreiferson, I proposed not tracking multiple topics per publisher, just one. Are you agreeing with that, or saying we should never track any topic for publisher stats (in this "first pass")?

mreiferson · 2017-04-11T18:41:51Z

@ploxiln I'm suggesting that, for now, perhaps it's easiest to land something that doesn't track the destination topic at all (but iterate on that after).

But, as I'm writing this, I'm realizing that really wouldn't be all that helpful for the use case here.

mreiferson · 2017-04-15T17:30:11Z

nsqd/client_v2.go

@@ -346,6 +353,10 @@ func (c *clientV2) SendingMessage() {
 	atomic.AddUint64(&c.MessageCount, 1)
 }

+func (c *clientV2) MessagesPublished(msgNum uint64) {


Let's call this PublishedMessage

mreiferson · 2017-04-15T17:32:14Z

nsqd/http.go

 	jsonFormat := formatString == "json"

+	var producersStats []ClientStats
+	if withProducers == "1" {


1 seems like an overly specific value... how about if it's a non-empty string instead?

It's sometimes confusing if "0" and "no" and "false" are considered "true", and awkward if you need to override a parameter with a blank string e.g. in your own helper functions. So I humbly suggest checking against "1", "yes", and "true", perhaps case-insensitive.

that's a fair point, 👍 to 1, yes, and true case-insensitive

However, with that, I would argue that the name of the param should be producers, not with_producers.

sparklxb · 2017-04-16T00:34:56Z

Ready for review and follow-up for nsqadmin. By the way, certificates under the nsqd/test directory has expired, so some test cases fail. please make a update.

ploxiln · 2017-04-16T01:12:22Z

nsqd/nsqd.go

+
+	_, ok := n.clients[clientID]
+	if !ok {
+		return


Unlock also needed here

mreiferson · 2017-04-16T22:05:18Z

nsqd/nsqd.go

+// AddClient adds a client to the producers map
+func (n *NSQD) AddClient(clientID int64, client Client) {
+	n.Lock()
+


whitespace 👮

mreiferson · 2017-04-16T22:05:26Z

nsqd/nsqd.go

+// RemoveClient removes a client from the producers map
+func (n *NSQD) RemoveClient(clientID int64) {
+	n.Lock()
+


whitespace 👮

mreiferson · 2017-04-16T22:05:52Z

This is looking good to me after those minor nitpicks.

ploxiln · 2017-04-17T23:02:38Z

mreiferson updated the certificates. Rebasing on master will fix those test failures.

mreiferson · 2017-10-02T08:23:27Z

I've rebased this, waiting for tests to pass.

mreiferson · 2017-10-02T08:32:41Z

Given that we're marching towards an actual 1.0 release, I want to be really careful of the new interfaces we add prior to that, since we won't have as much of a chance to work and play with them in production.

In that vein, after thinking more about this, it's certainly a good start but it seems like what one would really want is to understand which topics a producer is publishing too, right?

mreiferson · 2018-08-01T23:04:17Z

I've resurrected this PR, rebased, and implemented this from my last comment:

In that vein, after thinking more about this, it's certainly a good start but it seems like what one would really want is to understand which topics a producer is publishing too, right?

WDYT @sparklxb @ploxiln @jehiah ?

mreiferson · 2018-08-01T23:05:08Z

`/stats` output

nsqd v1.1.0-rc1 (built w/go1.10.3)
start_time 2018-08-01T15:52:49-07:00
uptime 24.680560278s

Health: OK

Memory:
   heap_objects                 8393
   heap_idle_bytes              360448
   heap_in_use_bytes            2195456
   heap_released_bytes          0
   gc_pause_usec_100            0
   gc_pause_usec_99             0
   gc_pause_usec_95             0
   next_gc_bytes                4473924
   gc_total_runs                0

Topics:
   [test           ] depth: 4     be-depth: 4     msgs: 0        e2e%:

   [test2          ] depth: 2     be-depth: 1     msgs: 1        e2e%:

   [test3          ] depth: 2     be-depth: 1     msgs: 1        e2e%:

Producers:     
   [V2 PROSNAKES            ] msgs: 1        connected: 10s
      [test2          ] msgs: 1

   [V2 PROSNAKES            ] msgs: 1        connected: 8s
      [test3          ] msgs: 1

`/stats?format=json` output

{
  "version": "1.1.0-rc1",
  "health": "OK",
  "start_time": 1533163969,
  "topics": [
    {
      "topic_name": "test",
      "channels": [],
      "depth": 4,
      "backend_depth": 4,
      "message_count": 0,
      "paused": false,
      "e2e_processing_latency": {
        "count": 0,
        "percentiles": null
      }
    },
    {
      "topic_name": "test2",
      "channels": [],
      "depth": 2,
      "backend_depth": 1,
      "message_count": 1,
      "paused": false,
      "e2e_processing_latency": {
        "count": 0,
        "percentiles": null
      }
    },
    {
      "topic_name": "test3",
      "channels": [],
      "depth": 2,
      "backend_depth": 1,
      "message_count": 1,
      "paused": false,
      "e2e_processing_latency": {
        "count": 0,
        "percentiles": null
      }
    }
  ],
  "memory": {
    "heap_objects": 9671,
    "heap_idle_bytes": 311296,
    "heap_in_use_bytes": 2244608,
    "heap_released_bytes": 0,
    "gc_pause_usec_100": 0,
    "gc_pause_usec_99": 0,
    "gc_pause_usec_95": 0,
    "next_gc_bytes": 4473924,
    "gc_total_runs": 0
  },
  "producers": [
    {
      "client_id": "PROSNAKES",
      "hostname": "PROSNAKES.local",
      "version": "V2",
      "remote_address": "127.0.0.1:61367",
      "state": 0,
      "ready_count": 0,
      "in_flight_count": 0,
      "message_count": 0,
      "finish_count": 0,
      "requeue_count": 0,
      "connect_ts": 1533163986,
      "sample_rate": 0,
      "deflate": false,
      "snappy": false,
      "user_agent": "to_nsq/1.1.0-rc1 go-nsq/1.0.6",
      "pub_counts": [
        {
          "topic": "test3",
          "count": 1
        }
      ],
      "tls": false,
      "tls_cipher_suite": "",
      "tls_version": "",
      "tls_negotiated_protocol": "",
      "tls_negotiated_protocol_is_mutual": false
    }
  ]
}

I am now the owner

ploxiln · 2018-08-02T00:00:37Z

nsqd/client_v2.go

+func (c *clientV2) PublishedMessage(topic string, count uint64) {
+	c.metaLock.Lock()
+	counter, ok := c.pubCounts[topic]
+	if ok {


looking up a missing key in a golang map returns the zero value ... it would work, but would it be bad style, to just:

c.pubCounts[topic] += count

ploxiln · 2018-08-02T00:10:45Z

overall looks great

mreiferson · 2018-08-02T17:46:39Z

I'll squash this down, but I don't want to land this until we stamp stable 1.1.0.

ploxiln · 2018-08-02T18:13:37Z

you mean "stable 1.1.0" right :)
sounds good

ploxiln · 2018-08-02T18:15:14Z

there seems to be a possible data race

WARNING: DATA RACE
Read at 0x00c420320330 by goroutine 85:
  github.com/nsqio/nsq/nsqd.(*Channel).flush()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/channel.go:212 +0xb50
  github.com/nsqio/nsq/nsqd.(*Channel).exit()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/channel.go:182 +0x3dc
  github.com/nsqio/nsq/nsqd.(*Channel).Close()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/channel.go:147 +0x3d
  github.com/nsqio/nsq/nsqd.(*Topic).exit()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/topic.go:373 +0x4dd
  github.com/nsqio/nsq/nsqd.(*Topic).Close()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/topic.go:335 +0x3d
  github.com/nsqio/nsq/nsqd.(*NSQD).Exit()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/nsqd.go:497 +0x298
  github.com/nsqio/nsq/nsqd.TestTouch()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/protocol_v2_test.go:645 +0xdb5
  testing.tRunner()
      /home/travis/.gimme/versions/go1.8.7.linux.amd64/src/testing/testing.go:657 +0x107
Previous write at 0x00c420320330 by goroutine 49:
  runtime.mapdelete()
      /home/travis/.gimme/versions/go1.8.7.linux.amd64/src/runtime/hashmap.go:598 +0x0
  github.com/nsqio/nsq/nsqd.(*Channel).popInFlightMessage()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/channel.go:468 +0x202
  github.com/nsqio/nsq/nsqd.(*Channel).FinishMessage()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/channel.go:350 +0x50
  github.com/nsqio/nsq/nsqd.(*protocolV2).FIN()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/protocol_v2.go:687 +0x457
  github.com/nsqio/nsq/nsqd.(*protocolV2).Exec()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/protocol_v2.go:177 +0x13d2
  github.com/nsqio/nsq/nsqd.(*protocolV2).IOLoop()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/protocol_v2.go:84 +0x634
  github.com/nsqio/nsq/nsqd.(*tcpServer).Handle()
      /home/travis/gopath/src/github.com/nsqio/nsq/nsqd/tcp.go:43 +0x86f

mreiferson · 2018-08-02T18:21:47Z

you mean "stable 1.1.0" right :)

yep, edited

there seems to be a possible data race

I'll take a look

mreiferson · 2018-08-02T18:45:12Z

It does look like there is indeed a race, but it's in an entirely unrelated code path. It's interesting that we've never observed it before, and that does indicate that this change might have made it more likely, but then again it did happen on an i386 and go 1.8.3 build.

IMO we can fix it separately if it continues to rear its ugly head.

ploxiln · 2018-08-02T18:52:32Z

agreed, we can fix separately if we ever see it again

sparklxb mentioned this pull request Apr 10, 2017

nsqd/nsqadmin: introspection of producer connections #879

Open

mreiferson changed the title ~~nsqd/nsqdmin: introspection of producer connections~~ nsqd/nsqadmin: introspection of producer connections Apr 11, 2017

mreiferson added the feature label Apr 11, 2017

mreiferson requested changes Apr 11, 2017

View reviewed changes

ploxiln reviewed Apr 11, 2017

View reviewed changes

mreiferson reviewed Apr 15, 2017

View reviewed changes

ploxiln reviewed Apr 16, 2017

View reviewed changes

mreiferson reviewed Apr 16, 2017

View reviewed changes

sparklxb force-pushed the master branch from f4e36a9 to 41ba852 Compare April 16, 2017 23:07

mreiferson force-pushed the master branch from 41ba852 to 5126133 Compare October 2, 2017 08:20

mreiferson previously approved these changes Oct 2, 2017

View reviewed changes

mreiferson changed the title ~~nsqd/nsqadmin: introspection of producer connections~~ nsqd: add producer client connections to /stats Jul 31, 2018

mreiferson force-pushed the master branch from 5126133 to a32fff3 Compare August 1, 2018 23:03

ploxiln reviewed Aug 2, 2018

View reviewed changes

mreiferson and others added 3 commits August 2, 2018 10:52

nsqd: cleanup unnecessary use of io.WriteString

581bad4

nsqd: cleanup some superfluous comments in http.go

8c9a66a

nsqd: introspection of producer connections

996be6d

mreiferson force-pushed the master branch from 756dfac to 996be6d Compare August 2, 2018 17:54

mreiferson merged commit e0e08a2 into nsqio:master Aug 20, 2018

nsqd: add producer client connections to /stats #881

nsqd: add producer client connections to /stats #881

Conversation

sparklxb commented Apr 10, 2017

ploxiln commented Apr 10, 2017

sparklxb commented Apr 11, 2017

mreiferson commented Apr 11, 2017

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

sparklxb Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln commented Apr 11, 2017 • edited Loading

mreiferson commented Apr 11, 2017

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

sparklxb commented Apr 16, 2017

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

mreiferson commented Apr 16, 2017

ploxiln commented Apr 17, 2017

mreiferson commented Oct 2, 2017

mreiferson commented Oct 2, 2017 • edited Loading

mreiferson commented Aug 1, 2018

mreiferson commented Aug 1, 2018

/stats output

/stats?format=json output

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln commented Aug 2, 2018

mreiferson commented Aug 2, 2018 • edited Loading

ploxiln commented Aug 2, 2018

ploxiln commented Aug 2, 2018

mreiferson commented Aug 2, 2018

mreiferson commented Aug 2, 2018 • edited Loading

ploxiln commented Aug 2, 2018

sparklxb Apr 11, 2017 •

edited

Loading

ploxiln Apr 11, 2017 •

edited

Loading

ploxiln commented Apr 11, 2017 •

edited

Loading

mreiferson commented Oct 2, 2017 •

edited

Loading

`/stats` output

`/stats?format=json` output

mreiferson commented Aug 2, 2018 •

edited

Loading

mreiferson commented Aug 2, 2018 •

edited

Loading