I realize we're already a bit wed to tailscale.com/util/clientmetric in order to get metrics out of tailscale, but I don't think we should add to the problem.

I admit that it was the reason why I decided to expose agent metrics this way. There is a function called collectMetrics(), defined in metrics.go, that reviews tailscale clientmetrics and attaches them to the Stats object.

I don't mind enabling a custom Prometheus registry in the agent to collect these metrics. It requires more implementation on our side, so it convinced me to stick with tailscale.com/util/clientmetric.

I will try to refactor this to use the registry 👍 .

I refactored the source to use PrometheusRegistry. It resulted in implementing a custom handler to merge Tailscale and agent metrics. Let me know your thoughts, and in the meantime, I will proceed with testing.

Unfortunately, the size of PR jumped from ~160 to ~450 LOC. I guess that I can limit it but providing some wrapper for prometheus.NewCounter + registerer.MustRegister.

@spikecurtis I refactored the source code to use metric labels. Looking at the code now (although it is still a draft), I'm not convinced about the error-wrapping technique.

Let me challenge it. There are cases where we'd like to collect more than one error, and trying to aggregate a few of them requires extra logic (at least append operation).

Consider the following code:

	if !isQuietLogin(session.RawCommand()) {
		manifest := s.Manifest.Load()
		if manifest != nil {
			err := showMOTD(session, manifest.MOTDFile)
			if err != nil {
				s.logger.Error(ctx, "show MOTD", slog.Error(err))
				s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, "yes", "motd").Add(1)
			}
		} else {
			s.logger.Warn(ctx, "metadata lookup failed, unable to show MOTD")
		}
	}

We'd like to know that something went wrong with MOTD, but it doesn't mean that the session gets terminated instantly. If something bad happens to the session later, the code will have to bubble up a sum of errors. Personally, I don't see a big gain here compared to a simple intercepting one-liner:

s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, "yes", "motd").Add(1)

I'm not convinced about the error-wrapping technique.

Let me challenge it. There are cases where we'd like to collect more than one error, and trying to aggregate a few of them requires extra logic (at least append operation).

That's fair---the high level goal is that each legitimately distinct error is counted exactly once. For errors that don't terminate the session, I think it's fine to just directly increment the error counter and move on.

What I want to avoid though, is double-counting, where an error type is incremented, and then a "generic error" count is also incremented after the function returns. We could limit the error wrapping to just those errors that trigger an immediate return in the session handler. The feature I really like about error wrapping is that it affords us the ability to ensure that we really are counting all the session-ending errors and not just forgetting in some return paths.

I see your point, but considering the fact that we have to increase the error counter in two different ways, the argumentation does not convince me.

What I want to avoid though, is double-counting, where an error type is incremented, and then a "generic error" count is also incremented after the function returns.

It is a theoretical use case, as most crucial code paths fail fast and return (connection is terminated). There is a low chance that the same counter is incremented twice.

The feature I really like about error wrapping is that it affords us the ability to ensure that we really are counting all the session-ending errors and not just forgetting in some return paths.

This is a valid point, but in my opinion, it complicates the code logic too much - I suppose that this is a matter of personal preference.

I'm wondering if we can push this change in the follow-up iteration to deliver the feature first. What you're suggesting here is pure refactoring. Nevertheless, I will start doing it now 👍 .

It is a theoretical use case, as most crucial code paths fail fast and return (connection is terminated). There is a low chance that the same counter is incremented twice.

I don't mean as some race condition --- I just mean counting an error at two different places in the stack. Your earlier PR incremented metrics both in sessionStart and then also in sessionHandler if it returned an error. Doesn't look like this PR does this any more, so it's fine not to implement error wrapping if you feel strongly about it.

LGTM!