Trace ssh sessions

[rosstimothy]

Adds a wrapper around ssh.Session which injects tracing context
in a similar manner to the ssh.Client wrapper.
All usages of ssh.Session have now been replaced and have the appropriate
context.Context passed along

Part of #12241

[ryanclark]

This is duplicated quite a few times, with the only things changing are what's in line #365 and #371, and some having extra attributes - whats your thoughts on abstracting it out?

Something like

func StartSSHTrace(ctx context.Context, kind, name string, tracer *tracing.Provider, attributes ...attribute.KeyValue) (*context.Context, trace.error) {
	return tracer.Start(
		ctx,
		fmt.Sprintf("ssh.%s/%s", kind, name),
		oteltrace.WithSpanKind(oteltrace.SpanKindClient),
		oteltrace.WithAttributes(
			append(
				semconv.RPCServiceKey.String("ssh.Client"),
				semconv.RPCMethodKey.String(kind),
				semconv.RPCSystemKey.String("ssh"),
				attributes...,
			)...,
		),
	)
}

[rosstimothy]

I did toy around with this, but I'm not sure that it adds much value or makes things any easier or less verbose.

I ended up with a slightly different function than the one your proposed:

func newSpan(ctx context.Context, name string, cfg *tracing.Config, attributes ...attribute.KeyValue) (context.Context, oteltrace.Span) {
	tracer := cfg.TracerProvider.Tracer(instrumentationName)

	return tracer.Start(
		ctx,
		name,
		oteltrace.WithSpanKind(oteltrace.SpanKindClient),
		oteltrace.WithAttributes(semconv.RPCSystemKey.String("ssh")),
		oteltrace.WithAttributes(
			attributes...,
		),
	)
}

Which turns the call site into:

	ctx, span := newSpan(
		ctx,
		fmt.Sprintf("ssh.SessionRequest/%s", name),
		config,
		[]attribute.KeyValue{
			attribute.Bool("want_reply", wantReply),
			semconv.RPCServiceKey.String("ssh.Session"),
			semconv.RPCMethodKey.String("SendRequest"),
		}...,
	)

[ryanclark]

Yeah I was hesitant to suggest it, as I realise it's still pretty verbose - I was just thinking of keeping similar attributes the same across the board. Happy either way!

[ryanclark]

This could be a sync.RWMutex

[fspmarshall]

RWMutexes aren't more efficient unless you expect fairly high numbers of concurrent reads, both in absolute, and relative to the number of concurrent writes. Near as I can tell that isn't true for this type.

[ryanclark]

& then this becomes

Suggested change

	c.lock.Lock()
	defer c.lock.Unlock()
	c.lock.RLock()
	defer c.lock.RUnlock()

[fspmarshall]

This function modifies c.contexts.

[fspmarshall]

LGTM. I think if we add any more instrumentation to the ssh types we might want to consider creating interfaces to abstract over them. Feels fiddly to have to change the types all over to add a wrapper.

[github-actions]

@rosstimothy See the table below for backport results.

Branch	Result
branch/v10	Create PR