Thanks for working on this and this is awesome! I just have some minor style nits and rest looks all good from SQL Obs 👍

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @lidorcarmel, and @nvanbenschoten)

pkg/kv/txn.go, line 128 at r1 (raw file):

	now := db.clock.NowAsClockTimestamp()
	kvTxn := roachpb.MakeTransaction("unnamed", nil, roachpb.NormalUserPriority, now.ToTimestamp(),
		db.clock.MaxOffset().Nanoseconds(), int32(db.ctx.NodeID.SQLInstanceID()))

nit: let's keep the original formatting here?

	kvTxn := roachpb.MakeTransaction(
		"unnamed",
		nil, // baseKey
		roachpb.NormalUserPriority,
		now.ToTimestamp(),
		db.clock.MaxOffset().Nanoseconds(),
                int32(db.ctx.NodeID.SQLInstanceID()),
	)

pkg/kv/kvclient/kvcoord/dist_sender_test.go, line 2171 at r1 (raw file):

	ds := NewDistSender(cfg)

	txn := roachpb.MakeTransaction("foo", nil, 1.0, clock.Now(), 0, 1)

nit: let's add the inline /* sqlNodeID */ comment here

pkg/kv/kvserver/replica_test.go, line 4552 at r1 (raw file):

	keyB := roachpb.Key("b")
	// Start a transaction early to get a low timestamp.
	txn := roachpb.MakeTransaction("test", keyA, roachpb.NormalUserPriority, tc.Clock().Now(), 0 /* maxOffsetNs */, 0 /* sqlNodeID */)

small nit: wrap the long line here ?

Nice! I haven't made my way through all of the files that changed yet, but left a few comments on the important ones.

Reviewed 13 of 51 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @Azhng, and @lidorcarmel)

pkg/kv/txn.go, line 120 at r1 (raw file):

//
// See also db.NewTxn().
func NewTxn(ctx context.Context, db *DB, gatewayNodeID roachpb.NodeID) *Txn {

We had discussed not needing this gatewayNodeID param anymore in any of these constructors if/when we make it possible to determine whether SQLInstanceID is a reference to a KV node or to a SQL tenant node. Do you mind opening an issue to track that refactor?

pkg/kv/txn.go, line 128 at r1 (raw file):

+1

pkg/roachpb/data.go, line 916 at r1 (raw file):

// write conflicts in a way that avoids starvation of long-running
// transactions (see Replica.PushTxn).
// TODO(sarkesian): Write comment about sqlNodeID

Do you mind addressing this TODO in this PR?

pkg/sql/logictest/logic.go, line 281 at r1 (raw file):

//    Introduces a sleep period. Example: sleep 2s
//
//  - user <username> <nodeidx=N>

nit: [nodeidx=N]

pkg/storage/enginepb/mvcc3.proto, line 126 at r1 (raw file):

  reserved 8;

  // The node ID of the SQL node where this transaction originated.

What do you think about calling this either CoordinatorNodeID or GatewayNodeID? I'm not sure that SQLNodeID implies the right thing if this is a transaction that is being started by a KV node.

This actually raises a few interesting questions. What exactly is this representing? And what should it represent?

Currently, the transaction coordinator (TxnCoordSender) is always the same as the SQL gateway that uses the transaction. But you could imagine an architecture where this is not the case. For instance, in a reimagined architecture, it could be possible for the SQL executor to use a remote TxnCoordSender. So if these were decoupled, which component would this be a reference to? I think the answer is the "gateway" and not the "coordinator", because that's what houses the transaction's SQL metadata.

However, you could also imagine an architecture where the KV layer might want to know about the coordinator node ID of a given transaction, because it might want to talk to it directly. For instance, we may want a transaction that aborts some other transaction to reach out to its coordinator and inform it directly, instead of waiting an arbitrary amount of time for the aborted transaction to find out that it's aborted. So this might be an opportune time to begin storing the coordinator ID in intents.

What do you think?

pkg/storage/enginepb/mvcc3.proto, line 130 at r1 (raw file):

  // representing either a SQLInstanceID of a SQL pod or a gateway NodeID,
  // and is utilized for SQL Observability.
  // TODO(sarkesian): Refactor/include gogoproto.casttype when #73309 completes.

Do you mind mentioning which type we plan to use? Is it GenericNodeID?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @lidorcarmel, and @nvanbenschoten)

pkg/kv/txn.go, line 120 at r1 (raw file):

OK - created #73871.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @Azhng, @lidorcarmel, and @nvanbenschoten)

pkg/kv/kvserver/replica_test.go, line 4552 at r1 (raw file):

Done.

pkg/roachpb/data.go, line 916 at r1 (raw file):

Ha! Thanks for catching, I forgot to come back to this.

pkg/storage/enginepb/mvcc3.proto, line 126 at r1 (raw file):

After speaking directly we agreed to use CoordinatorNodeID as the field name... it should be noted that if we do have a scenario in which a SQL executor uses a remote TxnCoordSender, it would be necessary (for the purposes of SQL Observability) to have this field represent the node of the SQL executor, and if we wanted to keep track of the KV coordinator node, we'd need a separate field. It sounds like this scenario has been discussed and decided against previously, however, so using the more generic name CoordinatorNodeID should be fine.

pkg/storage/enginepb/mvcc3.proto, line 130 at r1 (raw file):

Also followed up on this directly - we agreed this would be a GenericNodeID.

Reviewed 27 of 51 files at r1, 1 of 3 files at r2, 18 of 20 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @Azhng, and @lidorcarmel)

pkg/sql/logictest/testdata/logic_test/contention_event, line 80 at r5 (raw file):

true

# check that the sql instance id / gateway node id is properly stored with the contention event

nit: this comment needs a small update to use the new name.