feat: support NATS #270

mannkafai · 2025-01-16T15:43:27Z

This will close #206

vercel · 2025-01-16T15:43:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
kyanos	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 18, 2025 3:05pm

bpf/protocol_inference.h

mannkafai · 2025-01-18T14:35:04Z

@hengyoush All the tests are stuck in Test filter by l3/l4 info. There are multiple IPs returned when executing dig example.com +short. Can you help me take a look?

+ test_filter_by_remote_ip
++ dig example.com +short
+ remote_ip='23.192.228.84
23.215.0.136
23.215.0.138
96.7.128.175
96.7.128.198
23.192.228.80'

hengyoush · 2025-01-19T15:22:38Z

agent/protocol/nats/nats.go

+	if !ok1 || !ok2 {
+		return []protocol.Record{}
+	}
+	return protocol.MatchByTimestamp(reqStream, respStream)


According to the NATS protocol, not all requests necessarily have responses (https://docs.nats.io/reference/reference-protocols/nats-protocol#ok-err). Is it possible that solely using MatchByTimestamp might result in losing records?

Yes, you are right. Only [H]PUB or [H]MSG with reply-to and PING/PONG are matching request/response. Both NATS server and client could start with PING, I'm not sure it's possible to distinguish which side starts with PING.

I'm not sure it's possible to distinguish which side starts with PING.

I wonder if it is really necessary to distinguish which side initiated the ping. For ping/pong, can we match them using the MatchByTimestamp logic?

For other types of messages, we can traverse both respStream and reqStream simultaneously. If the respMessage is ok/err, then it forms a record with the reqMessage.
If the respMessage is not ok/err, then we need to output a record based on the timestamps of the current reqMessage and respMessage. If reqMessage.Ts < respMessage.Ts, output a record with only the req (resp is empty); otherwise, output a record with only the resp.

hengyoush · 2025-01-19T15:26:16Z

agent/protocol/nats/nats.go

+}
+
+func (parser *NatsStreamParser) FindBoundary(streamBuffer *buffer.StreamBuffer, messageType protocol.MessageType, startPos int) int {
+	return 0


Maybe we can determine the boundary based on the message type (nats protocol messages) (actually what you did in protocol_inference.h)

ok, I will fix this

hengyoush · 2025-01-19T15:27:33Z

agent/protocol/nats/nats.go

+		}
+	}
+	offset := index - len(method)
+	msg, prasedLen := natsParser.Parse(buffer[offset:], messageType)


typo here: prasedLen => parsedLen

hengyoush · 2025-01-19T15:31:33Z

agent/protocol/nats/nats.go

+	if packetLen < 0 {
+		return nil, -1
+	}
+	parts := splitFields(payload[:packetLen-len(_CRLF_)])


Do we need to check slice bound here?(len(payload) < packetLen-len(_CRLF_))

It's no necessary. packetLen means the first line length in payload(if contains \r\n), len(payload) >= packetLen always true.

hengyoush · 2025-01-19T15:38:17Z

bpf/protocol_inference.h

@@ -127,6 +127,141 @@ static __always_inline enum message_type_t is_http_protocol(const char *old_buf,
  return kUnknown;
 }

+static __always_inline bool is_nats_info(const char *old_buf, size_t count) {
+  if (count < 6)


Is this check for less than 6 redundant, since 20 bytes will be read below?

hengyoush · 2025-01-19T15:38:26Z

bpf/protocol_inference.h

+}
+
+static __always_inline bool is_nats_connect(const char *old_buf, size_t count) {
+  if (count < 8)


hengyoush · 2025-01-19T15:42:12Z

bpf/protocol_inference.h

+  bpf_probe_read_user(buf, 20, old_buf);
+
+#pragma unroll
+  for (size_t p = 5; p < 20; p++) {


Can this loop check be omitted? If so, we can determine nats message type with just one bpf_probe_read_user call to reduce the num of instructions.

I'm not sure is other protocols start whit INFO or CONNECT as NATS did, if not I think this loop can be omitted.

hengyoush · 2025-01-19T15:46:05Z

@hengyoush All the tests are stuck in Test filter by l3/l4 info. There are multiple IPs returned when executing dig example.com +short. Can you help me take a look?

Indeed, this need to be fixed with adding a tail/head -n 1, I have fixed in main branch.

ccoVeille

I TBI k I could continue but it's a good start

ccoVeille · 2025-01-19T15:58:51Z

agent/protocol/nats/filter.go

+	"kyanos/agent/protocol"
+	"kyanos/bpf"
+	"kyanos/common"
+	"slices"


Suggested change

"kyanos/agent/protocol"

"kyanos/bpf"

"kyanos/common"

"slices"

"slices"

"kyanos/agent/protocol"

"kyanos/bpf"

"kyanos/common"

ccoVeille · 2025-01-19T15:59:57Z

agent/protocol/nats/nats.go

+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"kyanos/agent/buffer"
+	"kyanos/agent/protocol"
+	"kyanos/bpf"
+	"kyanos/common"
+	"strconv"
+	"strings"
+)


Please fix import orders

ccoVeille · 2025-01-19T16:00:23Z

agent/protocol/nats/nats.go

+)
+
+const (
+	_CRLF_ = "\r\n"


We don't use uppercase constant in Go

ccoVeille · 2025-01-19T16:01:18Z

agent/protocol/nats/nats.go

+	if msg.ProtocolCode == PING || msg.ProtocolCode == PONG {
+		return 1
+	}
+	return 0


This could be a switch

ccoVeille · 2025-01-19T16:02:53Z

agent/protocol/nats/nats.go

+	// if strings.ToUpper(string(parts[0])) != "INFO" {
+	// 	return nil, packetLen
+	// }


Commented code is always suspicious.

Either it could have been removed, or something was temporarily commented for debug and it was left commented while it shouldn't

ccoVeille · 2025-01-19T16:16:26Z

agent/protocol/nats/nats.go

+
+func (m *Hmsg) Parse(payload []byte, messageType protocol.MessageType) (*NatsMessage, int) {
+	common.ProtocolParserLog.Debugf("NATS Parse Hmsg:%d, %x", len(payload), string(payload))
+	msg, prasedLen := m.ParseData(payload)


ccoVeille · 2025-01-19T16:16:41Z

agent/protocol/nats/nats.go

+	// if !strings.HasPrefix(strings.ToUpper(string(payload[:4])), "PING") {
+	// 	return nil, packetLen
+	// }


ccoVeille · 2025-01-19T16:18:24Z

cmd/nats.go

+}
+
+func init() {
+	natsCmd.Flags().StringSlice("protocols", []string{}, "Specify the nats protocol to monitor(PUB, SUB, MSG), seperate by ','")


Suggested change

natsCmd.Flags().StringSlice("protocols", []string{}, "Specify the nats protocol to monitor(PUB, SUB, MSG), seperate by ','")

natsCmd.Flags().StringSlice("protocols", []string{}, "Specify the NATS protocol to monitor (PUB, SUB, MSG), seperate by ','")

ccoVeille · 2025-01-19T16:19:58Z

cmd/nats.go

+
+func init() {
+	natsCmd.Flags().StringSlice("protocols", []string{}, "Specify the nats protocol to monitor(PUB, SUB, MSG), seperate by ','")
+	natsCmd.Flags().StringSlice("subjects", []string{}, "Specify the nats subject to monitor, seperate by ','")


Suggested change

natsCmd.Flags().StringSlice("subjects", []string{}, "Specify the nats subject to monitor, seperate by ','")

natsCmd.Flags().StringSlice("subjects", []string{}, "Specify the NATS subject to monitor, seperate by ','")

ccoVeille · 2025-01-19T16:20:44Z

testdata/nats/nats_echo.go

+	interval := flag.Duration("interval", 5*time.Second, "Interval between messages")
+	flag.Parse()
+
+	// 连接到NATS服务器


Please use plain English comments

feat: support NATS

79b7afc

vercel bot deployed to Preview January 16, 2025 15:43 View deployment

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Jan 16, 2025

hengyoush reviewed Jan 17, 2025

View reviewed changes

bpf/protocol_inference.h Outdated Show resolved Hide resolved

hengyoush reviewed Jan 17, 2025

View reviewed changes

bpf/protocol_inference.h Outdated Show resolved Hide resolved

fix erros

76840c6

vercel bot deployed to Preview January 17, 2025 15:34 View deployment

fix bpf error

e254880

vercel bot deployed to Preview January 18, 2025 13:38 View deployment

fix test_filter_by_remote_ip, pick the first ip

02bc852

vercel bot deployed to Preview January 18, 2025 15:05 View deployment

hengyoush reviewed Jan 19, 2025

View reviewed changes

ccoVeille reviewed Jan 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support NATS #270

feat: support NATS #270

mannkafai commented Jan 16, 2025

vercel bot commented Jan 16, 2025 •

edited

Loading

mannkafai commented Jan 18, 2025

hengyoush Jan 19, 2025

mannkafai Jan 25, 2025

hengyoush Jan 25, 2025

hengyoush Jan 19, 2025 •

edited

Loading

mannkafai Jan 25, 2025

hengyoush Jan 19, 2025

hengyoush Jan 19, 2025

mannkafai Jan 25, 2025

hengyoush Jan 19, 2025

hengyoush Jan 19, 2025

hengyoush Jan 19, 2025 •

edited

Loading

mannkafai Jan 25, 2025

hengyoush commented Jan 19, 2025 •

edited

Loading

ccoVeille left a comment

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

ccoVeille Jan 19, 2025

	natsCmd.Flags().StringSlice("protocols", []string{}, "Specify the nats protocol to monitor(PUB, SUB, MSG), seperate by ','")
	natsCmd.Flags().StringSlice("protocols", []string{}, "Specify the NATS protocol to monitor (PUB, SUB, MSG), seperate by ','")

	natsCmd.Flags().StringSlice("subjects", []string{}, "Specify the nats subject to monitor, seperate by ','")
	natsCmd.Flags().StringSlice("subjects", []string{}, "Specify the NATS subject to monitor, seperate by ','")

feat: support NATS #270

Are you sure you want to change the base?

feat: support NATS #270

Conversation

mannkafai commented Jan 16, 2025

vercel bot commented Jan 16, 2025 • edited Loading

mannkafai commented Jan 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hengyoush Jan 19, 2025 • 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

hengyoush Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hengyoush commented Jan 19, 2025 • edited Loading

ccoVeille 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

vercel bot commented Jan 16, 2025 •

edited

Loading

hengyoush Jan 19, 2025 •

edited

Loading

hengyoush Jan 19, 2025 •

edited

Loading

hengyoush commented Jan 19, 2025 •

edited

Loading