[PR] Enhance log module and enhance address resolver in access log module [skywalking-rover]

["mrproliu (via GitHub)" <gi...@apache.org>]

mrproliu opened a new pull request, #213:
URL: https://github.com/apache/skywalking-rover/pull/213

   (no comment)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Enhance log module and enhance address resolver in access log module [skywalking-rover]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #213:
URL: https://github.com/apache/skywalking-rover/pull/213#discussion_r3587411137


##########
pkg/accesslog/common/connection.go:
##########
@@ -277,6 +443,163 @@ func (c *ConnectionManager) buildRemoteAddress(e *events.SocketConnectEvent, soc
 	return c.buildAddressFromRemote(socket.DestIP, socket.DestPort)
 }
 
+// recordConnectionResolveResult records the final remote address resolve result of the
+// connection, it MUST be called when the connection is being deleted from the manager,
+// at that point all resolvers are finalized: the conntrack rewriting happens on the
+// address building, and the ztunnel correlation could attach the real destination on
+// any later flush, so judging any earlier would mis-count the resolved connections
+func (c *ConnectionManager) recordConnectionResolveResult(connection *ConnectionInfo) {
+	if connection == nil || connection.RPCConnection == nil {
+		return
+	}
+	remote := connection.RPCConnection.GetRemote()
+	if remote == nil || remote.GetIp() == nil {
+		// the remote address is resolved to a local monitored process, not a raw IP
+		return
+	}
+	if connection.RPCConnection.GetAttachment() != nil {
+		// the ztunnel correlation attached the real destination
+		c.ztunnelResolvedRemoteCount.Add(1)
+		return
+	}
+	if connection.Socket != nil && connection.Socket.ConnTrackResolved {
+		// the conntrack query already rewrote the address to the real peer(e.g. pod IP),
+		// the address is sent as a raw IP but the backend could resolve it
+		c.conntrackResolvedRemoteCount.Add(1)
+		return
+	}
+	// ask the resolution-aware flusher(s) WHY this connection ended up unresolved, so the summary
+	// can group the raw-IP socket pairs by the environment/source that failed to provide a mapping
+	reason := "unresolved"
+	for _, l := range c.flushListeners {
+		if r, ok := l.(ResolutionAwareFlusher); ok {
+			if rs := r.UnresolvedReason(connection); rs != "" {
+				reason = rs
+				break
+			}
+		}
+	}
+	src := "unknown"
+	if connection.Socket != nil {
+		src = fmt.Sprintf("%s:%d", connection.Socket.SrcIP, connection.Socket.SrcPort)
+	}
+	c.recordUnresolvedRemote(reason, fmt.Sprintf("%s->%s:%d", src, remote.GetIp().GetHost(), remote.GetIp().GetPort()))
+}
+
+// recordUnresolvedRemote records a socket pair(src->dst) that cannot be resolved by any of the
+// resolvers(local process, conntrack, ztunnel correlation) - it would reach the backend as a raw
+// IP - together with the categorized reason, for the periodic summary. The per-reason totals are
+// always counted; the per-pair map is bounded so a flood of distinct pairs cannot grow it without
+// limit.
+func (c *ConnectionManager) recordUnresolvedRemote(reason, socketPair string) {
+	c.unresolvedRemoteCount.Add(1)
+	c.unresolvedRemoteLock.Lock()
+	defer c.unresolvedRemoteLock.Unlock()
+	c.unresolvedByReason[reason]++
+	key := reason + " " + socketPair
+	if _, exist := c.unresolvedRemotes[key]; !exist && len(c.unresolvedRemotes) >= unresolvedRemoteMaxTrack {
+		return
+	}
+	c.unresolvedRemotes[key]++
+}
+
+// drainUnresolvedRemotes returns, since the last report, the top un-resolved socket pairs(each
+// prefixed with its reason) and the full per-reason breakdown, and resets the tracking maps.
+func (c *ConnectionManager) drainUnresolvedRemotes() (topPairs, byReason string) {
+	c.unresolvedRemoteLock.Lock()
+	pairs := c.unresolvedRemotes
+	reasons := c.unresolvedByReason
+	c.unresolvedRemotes = make(map[string]int64)
+	c.unresolvedByReason = make(map[string]int64)
+	c.unresolvedRemoteLock.Unlock()
+
+	sortByCount := func(m map[string]int64, topN int) string {
+		if len(m) == 0 {
+			return "none"
+		}
+		type kv struct {
+			k string
+			v int64
+		}
+		sorted := make([]kv, 0, len(m))
+		for k, v := range m {
+			sorted = append(sorted, kv{k: k, v: v})
+		}
+		sort.Slice(sorted, func(i, j int) bool { return sorted[i].v > sorted[j].v })
+		if topN > 0 && len(sorted) > topN {
+			sorted = sorted[:topN]
+		}
+		items := make([]string, 0, len(sorted))
+		for _, s := range sorted {
+			items = append(items, fmt.Sprintf("%s=%d", s.k, s.v))
+		}
+		return strings.Join(items, ", ")
+	}
+	return sortByCount(pairs, unresolvedRemoteTopCount), sortByCount(reasons, 0)
+}
+
+// startUnresolvedRemoteReporter periodically reports the summary of remote addresses
+// that cannot be resolved(by neither the conntrack nor the ztunnel correlation) at the info level
+func (c *ConnectionManager) startUnresolvedRemoteReporter(ctx context.Context) {
+	go func() {
+		ticker := time.NewTicker(unresolvedRemoteReportInterval)
+		defer ticker.Stop()
+		var lastUnresolvedCount int64
+		for {
+			select {
+			case <-ticker.C:
+				unresolvedCount := c.unresolvedRemoteCount.Load()
+				if unresolvedCount == lastUnresolvedCount {
+					continue
+				}
+				lastUnresolvedCount = unresolvedCount
+				conntrackStats := "not available"
+				if c.connectTracker != nil {
+					conntrackStats = c.connectTracker.StatsString()
+				}
+				topPairs, byReason := c.drainUnresolvedRemotes()
+				log.Infof("remote address resolve summary: total remote addresses built: %d, "+
+					"resolved by conntrack: %d, resolved by ztunnel correlation: %d, unresolved(sent as raw IP): %d, "+
+					"conntrack stats: {%s}, unresolved by reason: {%s}, "+
+					"open->resolution latency buckets {<1s: %d, 1-5s: %d, 5-15s: %d, >=15s: %d, max: %dms}, "+
+					"top unresolved socket pairs since last report: %s",
+					c.remoteAddressBuildCount.Load(), c.conntrackResolvedRemoteCount.Load(),
+					c.ztunnelResolvedRemoteCount.Load(), unresolvedCount,
+					conntrackStats, byReason,
+					c.resolutionLatencyUnder1s.Load(), c.resolutionLatency1to5s.Load(),
+					c.resolutionLatency5to15s.Load(), c.resolutionLatencyOver15s.Load(),
+					c.resolutionLatencyMaxMilli.Load(), topPairs)

Review Comment:
   The periodic "remote address resolve summary" log mixes cumulative and interval metrics: `unresolvedRemoteCount` is cumulative since start, but `unresolved by reason` / `top unresolved socket pairs since last report` are drained-and-reset per interval. This makes the log line internally inconsistent and can mislead operational debugging (e.g., reasons/pairs appear to drop while the unresolved count only increases). Consider logging a per-interval delta (and optionally also the cumulative total) so all fields describe the same time window.



##########
pkg/tools/netns/netns.go:
##########
@@ -0,0 +1,68 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package netns
+
+import (
+	"fmt"
+	"os"
+	"runtime"
+
+	"golang.org/x/sys/unix"
+)
+
+// RunInNetNS executes fn with the current OS thread switched into the network
+// namespace referenced by netnsPath(e.g. /proc/<pid>/ns/net), and restores the
+// original network namespace afterwards.
+//
+// Requires CAP_SYS_ADMIN. Everything that must happen inside the target
+// namespace(creating sockets, dialing, reading responses) should be done
+// inside fn, since only the calling OS thread is switched.

Review Comment:
   The function comment says the original network namespace is restored afterwards, but on a restore failure `RunInNetNS` returns while intentionally keeping the OS thread locked. At that point the caller goroutine will continue running pinned to a thread that may still be in the target netns, which is a surprising and potentially dangerous contract. At minimum, document that a restore failure leaves the goroutine locked to its OS thread and it should exit promptly to avoid doing further work in the wrong namespace.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Enhance log module and enhance address resolver in access log module [skywalking-rover]

["mrproliu (via GitHub)" <gi...@apache.org>]

mrproliu merged PR #213:
URL: https://github.com/apache/skywalking-rover/pull/213


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org