allow limit to traced message #1057

andyxning · 2019-07-05T08:09:38Z

Resolves #None

Changes proposed in this pull request:

allow limit to traced message

/cc @nats-io/core

derekcollison

In general I like this idea. But need some changes.

derekcollison · 2019-07-05T18:08:42Z

server/client.go

+
+	origLength := len(msg) - LEN_CR_LF
+	validLength := origLength
+	if origLength > MAX_TRACED_MESSAGE_PRINTABLE_LEN {


This should be configurable.

derekcollison · 2019-07-05T18:09:11Z

server/client.go

+	if origLength > MAX_TRACED_MESSAGE_PRINTABLE_LEN {
+		validLength = MAX_TRACED_MESSAGE_PRINTABLE_LEN
+		if validLength >= 3 {
+			msg[validLength-3] = '.'


Should not write over msg, should just do a slice on original and then add '...'.

andyxning · 2019-07-11T04:06:08Z

@derekcollison What about this?

derekcollison · 2019-07-11T04:17:37Z

server/client.go

+
+	origLength := len(msg) - LEN_CR_LF
+	var msgToWrite []byte
+	if origLength > c.srv.opts.MaxTracedMessagePrintableLen {


opts should be grabbed under a lock, but only if we know we are tracing.

derekcollison · 2019-07-11T04:17:55Z

server/client.go

+	origLength := len(msg) - LEN_CR_LF
+	var msgToWrite []byte
+	if origLength > c.srv.opts.MaxTracedMessagePrintableLen {
+		msgToWrite = make([]byte, c.srv.opts.MaxTracedMessagePrintableLen+len("..."))


We should avoid allocation here IMO.

Otherwise, we need to trick the log formatter.

I am sure its possible and prefer a solution that does not do additional allocs if possible.

derekcollison · 2019-07-11T04:18:43Z

server/opts.go

@@ -190,6 +190,8 @@ type Options struct {
 	WriteDeadline    time.Duration `json:"-"`
 	MaxClosedClients int           `json:"-"`
 	LameDuckDuration time.Duration `json:"-"`
+	// MaxTracedMessagePrintableLen is the maximum printable length for traced messages.
+	MaxTracedMessagePrintableLen int `json:"max_traced_message_printable_len"`


Prefer shorter names here.

Any special concerns? I think it is more clear.

MaxTracedMsgLen

derekcollison · 2019-07-11T04:19:16Z

server/opts.go

@@ -2482,6 +2486,9 @@ func setBaselineOptions(opts *Options) {
 	if opts.ReconnectErrorReports == 0 {
 		opts.ReconnectErrorReports = DEFAULT_RECONNECT_ERROR_REPORTS
 	}
+	if opts.MaxTracedMessagePrintableLen == 0 {


Should we have something to designate no limit? -1 maybe?

Good catch. Will change default value for MaxTracedMessagePrintableLen to -1 to designate no limit.

andyxning · 2019-07-12T05:49:16Z

@derekcollison Comments addressed. PTAL.

derekcollison

Getting there. Need tests to show default value unlimited, so in code should check for > 0 too if we use -1.

derekcollison · 2019-07-12T14:56:41Z

server/client.go

+	origLength := len(msg) - LEN_CR_LF
+	validLength := origLength
+	formatter := "<<- MSG_PAYLOAD: [%q]"
+	if origLength > c.srv.getOpts().MaxTracedMsgLen {


only call getOpts once. opts := c.srv.getOpts()

server/reload.go

derekcollison · 2019-07-12T15:05:21Z

server/client.go

-	// FIXME(dlc), allow limits to printable payload.
-	c.Tracef("<<- MSG_PAYLOAD: [%q]", msg[:len(msg)-LEN_CR_LF])
+
+	origLength := len(msg) - LEN_CR_LF


Can't we just do..

if len(msg) - LEN_CR_LF > opts.MaxTracedMsgLen { c.Tracef("<<- MSG_PAYLOAD: [\"%s...\"]", msg[:opts.MaxTracedMsgLen]) } else { c.Tracef("<<- MSG_PAYLOAD: [%q]", msg[:len(msg)-LEN_CR_LF]) }

This way you do not need constants for formatters.

kozlovic · 2019-07-12T15:30:29Z

server/opts.go

@@ -190,6 +190,8 @@ type Options struct {
 	WriteDeadline    time.Duration `json:"-"`
 	MaxClosedClients int           `json:"-"`
 	LameDuckDuration time.Duration `json:"-"`
+	// MaxTracedMsgLen is the maximum printable length for traced messages.
+	MaxTracedMsgLen int `json:"max_traced_msg_len"`


We have deprecated use of json tags in Options because we used to use this directly in monitoring object but we no longer do.
As a side note, if this is something that we would want to see in /varz then it would have to be added in monitor.go. This can be done as separate PR though.

Done. I have use json:"-" for MaxTracedMsgLen.

andyxning · 2019-07-14T01:53:00Z

@kozlovic @derekcollison Comments addressed. PTAL.

derekcollison

Looking good. A few more changes I think needed.

derekcollison · 2019-07-14T02:08:22Z

server/opts.go

@@ -2482,6 +2486,9 @@ func setBaselineOptions(opts *Options) {
 	if opts.ReconnectErrorReports == 0 {
 		opts.ReconnectErrorReports = DEFAULT_RECONNECT_ERROR_REPORTS
 	}
+	if opts.MaxTracedMsgLen == -1 {
+		opts.MaxTracedMsgLen = MAX_TRACED_MSG_LEN


This feels too low for the default. The behavior now is to print the whole msg which I think we need to preserve.

Agree. I think we should not set baseline for MaxTracedMsgLen.

I have update the help info for max_traced_msg_len to add -1 for unlimited for describing that -1 is the default value for max_traced_msg_len and it defaults to unlimited. This should be ok for now.

derekcollison · 2019-07-14T02:09:34Z

server/opts.go

@@ -2551,6 +2558,7 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
 	fs.StringVar(&opts.TLSCert, "tlscert", "", "Server certificate file.")
 	fs.StringVar(&opts.TLSKey, "tlskey", "", "Private key for server certificate.")
 	fs.StringVar(&opts.TLSCaCert, "tlscacert", "", "Client certificate CA for verification.")
+	fs.IntVar(&opts.MaxTracedMsgLen, "max_traced_msg_len", -1, "Maximum printable length for traced messages")


This only sets via server binary. We have many people that embed the server in other Go apps so we need the default in that mode as well.

What is the way for lib users to initialize an instance for options? i have no idea about where to start?

I have thought about this:

Default value for max_traces_msg_len is 0. This should be also adapting to lib usage for unlimited message length

Print traced message with customized length only when MaxTracedMsgLen is 0.

@derekcollison PTAL, again.

derekcollison

In general I think we are looking good, still want to see some tests.

derekcollison · 2019-07-14T13:59:41Z

server/client.go

-	// FIXME(dlc), allow limits to printable payload.
-	c.Tracef("<<- MSG_PAYLOAD: [%q]", msg[:len(msg)-LEN_CR_LF])
+
+	opts := c.srv.getOpts()


could just do

maxTrace := c.srv.getOpts().MaxTracedMsgLen

derekcollison

LGTM

derekcollison · 2019-07-15T04:34:14Z

Thanks for your patience.

andyxning force-pushed the allow_limits_to_traced_message branch from 97c25cf to cdee4ab Compare July 5, 2019 08:31

derekcollison requested changes Jul 5, 2019

View reviewed changes

andyxning force-pushed the allow_limits_to_traced_message branch 8 times, most recently from 8050f92 to 10a4884 Compare July 10, 2019 09:20

derekcollison requested changes Jul 11, 2019

View reviewed changes

andyxning force-pushed the allow_limits_to_traced_message branch from 10a4884 to f700d21 Compare July 12, 2019 05:49

andyxning force-pushed the allow_limits_to_traced_message branch 3 times, most recently from 1f2df4f to 6ea0067 Compare July 12, 2019 07:50

derekcollison requested changes Jul 12, 2019

View reviewed changes

kozlovic reviewed Jul 12, 2019

View reviewed changes

andyxning force-pushed the allow_limits_to_traced_message branch from 6ea0067 to 1f52ac4 Compare July 14, 2019 01:51

andyxning force-pushed the allow_limits_to_traced_message branch from 1f52ac4 to 5b62b62 Compare July 14, 2019 02:00

derekcollison requested changes Jul 14, 2019

View reviewed changes

andyxning force-pushed the allow_limits_to_traced_message branch 2 times, most recently from e66055b to 0eaf2a3 Compare July 14, 2019 05:13

derekcollison requested changes Jul 14, 2019

View reviewed changes

allow limit to traced message

cd214fc

andyxning force-pushed the allow_limits_to_traced_message branch from 0eaf2a3 to cd214fc Compare July 15, 2019 03:39

derekcollison approved these changes Jul 15, 2019

View reviewed changes

derekcollison merged commit 7a3fb4e into nats-io:master Jul 15, 2019

andyxning deleted the allow_limits_to_traced_message branch July 15, 2019 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow limit to traced message #1057

allow limit to traced message #1057

andyxning commented Jul 5, 2019

derekcollison left a comment

derekcollison Jul 5, 2019

derekcollison Jul 5, 2019

andyxning commented Jul 11, 2019

derekcollison Jul 11, 2019

andyxning Jul 11, 2019

derekcollison Jul 11, 2019

andyxning Jul 11, 2019

derekcollison Jul 11, 2019

derekcollison Jul 11, 2019

andyxning Jul 11, 2019

derekcollison Jul 11, 2019

derekcollison Jul 11, 2019

andyxning Jul 11, 2019

andyxning commented Jul 12, 2019

derekcollison left a comment

derekcollison Jul 12, 2019

derekcollison Jul 12, 2019

derekcollison Jul 12, 2019

kozlovic Jul 12, 2019

andyxning Jul 14, 2019

andyxning commented Jul 14, 2019

derekcollison left a comment

derekcollison Jul 14, 2019

andyxning Jul 14, 2019

derekcollison Jul 14, 2019

andyxning Jul 14, 2019 •

edited

andyxning Jul 14, 2019

andyxning Jul 14, 2019

derekcollison left a comment

derekcollison Jul 14, 2019

andyxning Jul 15, 2019

derekcollison left a comment

derekcollison commented Jul 15, 2019

allow limit to traced message #1057

allow limit to traced message #1057

Conversation

andyxning commented Jul 5, 2019

Changes proposed in this pull request:

derekcollison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyxning commented Jul 11, 2019

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 Jul 12, 2019

derekcollison 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

andyxning commented Jul 14, 2019

derekcollison 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 Jul 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison left a comment

Choose a reason for hiding this comment

derekcollison commented Jul 15, 2019

andyxning Jul 14, 2019 •

edited