Skip to content

feat(metrics): measure protocol detection accuracy across all protocols #322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mayankpande88 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(metrics): measure protocol detection accuracy across all protocols #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions containers/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ type ActiveConnection struct {

parseFailCount int
protocolOverride l7.Protocol // non-zero = override eBPF-detected protocol
parseSucceeded bool // at least one successful parse seen on this connection
}

type ListenDetails struct {
Expand Down Expand Up @@ -1292,7 +1293,7 @@ func (c *Container) onL7RequestWithResult(pid uint32, fd uint64, timestamp uint6
// Empty payloads are normal and are not counted as failures.
if len(r.Payload) > 0 {
if parser.SawValidFrame() {
conn.parseFailCount = 0
c.trackParseOK(conn, r.Protocol)
} else {
c.trackParseFail(conn, pid, fd, r.Protocol)
}
Expand Down Expand Up @@ -1402,7 +1403,7 @@ func (c *Container) onL7RequestWithResult(pid uint32, fd uint64, timestamp uint6
if query == "" && r.Method != l7.MethodStatementClose {
c.trackParseFail(conn, pid, fd, r.Protocol)
} else {
conn.parseFailCount = 0
c.trackParseOK(conn, r.Protocol)
}
if trace != nil {
trace.PostgresQuery(query, r.Status.Error(), r.Duration)
Expand Down Expand Up @@ -1456,7 +1457,7 @@ func (c *Container) onL7RequestWithResult(pid uint32, fd uint64, timestamp uint6
if query == "" {
c.trackParseFail(conn, pid, fd, r.Protocol)
} else {
conn.parseFailCount = 0
c.trackParseOK(conn, r.Protocol)
}
if trace != nil {
trace.ClickhouseQuery(query, r.Status.Error(), r.Duration)
Expand All @@ -1468,7 +1469,7 @@ func (c *Container) onL7RequestWithResult(pid uint32, fd uint64, timestamp uint6
if op == "" {
c.trackParseFail(conn, pid, fd, r.Protocol)
} else {
conn.parseFailCount = 0
c.trackParseOK(conn, r.Protocol)
}
if trace != nil {
trace.ZookeeperRequest(op, arg, r.Status, r.Duration)
Expand Down Expand Up @@ -1497,11 +1498,25 @@ func (c *Container) trackParseFail(conn *ActiveConnection, pid uint32, fd uint64
conn.parseFailCount++
if conn.parseFailCount == parseFailThreshold {
conn.protocolOverride = protocolReclassified
// Counted once per connection, at the threshold, so the metric is a
// count of connections given up on rather than of failed parses.
ProtocolReclassifiedTotal.WithLabelValues(protocolLabel(proto)).Inc()
klog.Warningf("reclassified connection pid=%d fd=%d from %s to unknown after %d consecutive parse failures",
pid, fd, proto, conn.parseFailCount)
}
}

// trackParseOK records the denominator for the misdetection rate: the first
// successful parse on a connection. Counted once, so both sides of the ratio
// are per-connection rather than per-event.
func (c *Container) trackParseOK(conn *ActiveConnection, proto l7.Protocol) {
if !conn.parseSucceeded {
Comment on lines +1512 to +1513

@gemini-code-assist gemini-code-assist Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To adhere to defensive programming best practices, we should ensure that 'conn' is not 'nil' before accessing its properties ('parseSucceeded' and 'parseFailCount'). Adding a nil check prevents potential runtime panics if this helper is ever called with a nil connection pointer.

func (c *Container) trackParseOK(conn *ActiveConnection, proto l7.Protocol) {
	if conn == nil {
		return
	}
	if !conn.parseSucceeded {

@mayankpande88 mayankpande88 Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Declining this one, with reasoning.

conn cannot be nil at any trackParseOK call site. All five sit inside the protocol switch in onL7RequestWithResult, which is reached only after conn has been dereferenced repeatedly — conn.DestinationKey, conn.srcWorkload, conn.Timestamp — so a nil conn would have panicked well before this point.

The adjacent trackParseFail, which this mirrors and which has existed for some time, has no nil check either. Adding one to only the new function would be inconsistent, and adding a guard that cannot fire suggests the invariant is uncertain when it is not — it makes the code marginally harder to reason about rather than easier.

If nil-safety here is wanted as a policy, it should go on both functions in a separate change, so the reasoning applies uniformly rather than to whichever function happened to be touched most recently.

conn.parseSucceeded = true
ConnectionsParsedTotal.WithLabelValues(protocolLabel(proto)).Inc()
}
conn.parseFailCount = 0
}

// processHTTP2WithoutConnection handles HTTP/2 events when TCP connection tracking failed.
// This is common for Go TLS connections where goroutines switch threads between
// TCP connect and TLS write, causing fd_by_pid_tgid lookup to fail in eBPF.
Expand Down
34 changes: 34 additions & 0 deletions containers/llm_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,38 @@ var (
[]string{"bucket", "destination", "direction"},
)

// ProtocolReclassifiedTotal counts connections given up on after
// parseFailThreshold consecutive parse failures, and ConnectionsParsedTotal
// counts connections that produced at least one successful parse. Together
// they give a per-protocol misdetection rate.
//
// Detection accuracy was previously only observable for HTTP/2, via the
// invalid-frame ratio, and only because that ratio was instrumented while
// chasing a specific bug. Every protocol here is detected by a byte-pattern
// heuristic of similar strength — the ClickHouse check is three bytes — and
// each one caches its verdict on the connection, so the same failure mode
// that made HTTP/2 misclassify HTTPS/1.1 traffic is possible elsewhere and
// currently unmeasured.
//
// A reclassification is a definitive misdetection: the protocol was decided
// by eBPF and then contradicted by the parser refusing the payload
// repeatedly. Rate = reclassified / (reclassified + parsed).
ProtocolReclassifiedTotal = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "node_agent_protocol_reclassified_total",
Help: "Connections reclassified after repeated parse failures, by protocol",
},
[]string{"protocol"},
)

ConnectionsParsedTotal = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "node_agent_connections_parsed_total",
Help: "Connections that produced at least one successful parse, by protocol",
},
[]string{"protocol"},
)

// ContainerLLMCachedTokensTotal counts input tokens served from the
// provider's prompt cache. Already counted in token_usage_total{type=input};
// this is a separate metric to make cache-hit rate computable.
Expand Down Expand Up @@ -330,6 +362,8 @@ func RegisterLLMMetrics(reg prometheus.Registerer) {
Http2StageTotal,
Http2FramesTotal,
Http2PayloadSizeTotal,
ProtocolReclassifiedTotal,
ConnectionsParsedTotal,
ContainerLLMCachedTokensTotal,
ContainerLLMToolCallsTotal,
ContainerLLMCostUSDTotal,
Expand Down
Loading