Add failed_tunnel persistent stat

- failed_tunnel records dial params
  and tunnel dial/handshake/etc. error
  and reports via status requests once
  a tunnel is established.

- Update remote_server_list persistent
  stat to record and send IDs and
  version info as configured at
  store-time.

- Add new tactics to control persistent
  stat recording and sending. Sending
  cap is now specified in bytes. A limit
  is imposed on number of records stored.

- Cleanup session ID parameters, as session
  ID is now static per controller instance
  and always available via Config.

psiphon/common/parameters/clientParameters.go

@@ -202,6 +202,10 @@ const (
 	APIRequestUpstreamPaddingMaxBytes          = "APIRequestUpstreamPaddingMaxBytes"
 	APIRequestDownstreamPaddingMinBytes        = "APIRequestDownstreamPaddingMinBytes"
 	APIRequestDownstreamPaddingMaxBytes        = "APIRequestDownstreamPaddingMaxBytes"
+	PersistentStatsMaxStoreRecords             = "PersistentStatsMaxStoreRecords"
+	PersistentStatsMaxSendBytes                = "PersistentStatsMaxSendBytes"
+	RecordRemoteServerListPersistentStats      = "RecordRemoteServerListPersistentStats"
+	RecordFailedTunnelPersistentStats          = "RecordFailedTunnelPersistentStats"
 )
 
 const (
@@ -334,7 +338,9 @@ var defaultClientParameters = map[string]struct {
 	PsiphonAPIStatusRequestPeriodMax:      {value: 10 * time.Minute, minimum: 1 * time.Second},
 	PsiphonAPIStatusRequestShortPeriodMin: {value: 5 * time.Second, minimum: 1 * time.Second},
 	PsiphonAPIStatusRequestShortPeriodMax: {value: 10 * time.Second, minimum: 1 * time.Second},
-	PsiphonAPIPersistentStatsMaxCount:     {value: 100, minimum: 1},
+	// PsiphonAPIPersistentStatsMaxCount parameter is obsoleted by PersistentStatsMaxSendBytes.
+	// TODO: remove once no longer required for older clients.
+	PsiphonAPIPersistentStatsMaxCount: {value: 100, minimum: 1},
 	// PsiphonAPIStatusRequestPadding parameters are obsoleted by APIRequestUp/DownstreamPadding.
 	// TODO: remove once no longer required for older clients.
 	PsiphonAPIStatusRequestPaddingMinBytes: {value: 0, minimum: 0},
@@ -409,6 +415,11 @@ var defaultClientParameters = map[string]struct {
 	APIRequestUpstreamPaddingMaxBytes:   {value: 1024, minimum: 0},
 	APIRequestDownstreamPaddingMinBytes: {value: 0, minimum: 0},
 	APIRequestDownstreamPaddingMaxBytes: {value: 1024, minimum: 0},
+
+	PersistentStatsMaxStoreRecords:        {value: 200, minimum: 1},
+	PersistentStatsMaxSendBytes:           {value: 65536, minimum: 1},
+	RecordRemoteServerListPersistentStats: {value: true},
+	RecordFailedTunnelPersistentStats:     {value: true},
 }
 
 // IsServerSideOnly indicates if the parameter specified by name is used

psiphon/controller.go

@@ -46,7 +46,6 @@ import (
 // route traffic through the tunnels.
 type Controller struct {
 	config                                  *Config
-	sessionId                               string
 	runCtx                                  context.Context
 	stopRunning                             context.CancelFunc
 	runWaitGroup                            *sync.WaitGroup
@@ -106,7 +105,6 @@ func NewController(config *Config) (controller *Controller, err error) {
 
 	controller = &Controller{
 		config:       config,
-		sessionId:    config.SessionID,
 		runWaitGroup: new(sync.WaitGroup),
 		// connectedTunnels and failedTunnels buffer sizes are large enough to
 		// receive full pools of tunnels without blocking. Senders should not block.
@@ -1519,10 +1517,7 @@ func (controller *Controller) doFetchTactics(
 	}
 	defer meekConn.Close()
 
-	apiParams := getBaseAPIParameters(
-		controller.config,
-		controller.sessionId,
-		dialParams)
+	apiParams := getBaseAPIParameters(controller.config, dialParams)
 
 	tacticsRecord, err := tactics.FetchTactics(
 		ctx,
@@ -1891,7 +1886,6 @@ loop:
 		tunnel, err := ConnectTunnel(
 			controller.establishCtx,
 			controller.config,
-			controller.sessionId,
 			candidateServerEntry.adjustedEstablishStartTime,
 			dialParams)
 

psiphon/dataStore.go

@@ -39,6 +39,7 @@ var (
 	datastoreUrlETagsBucket                     = []byte("urlETags")
 	datastoreKeyValueBucket                     = []byte("keyValues")
 	datastoreRemoteServerListStatsBucket        = []byte("remoteServerListStats")
+	datastoreFailedTunnelStatsBucket            = []byte("failedTunnelStats")
 	datastoreSLOKsBucket                        = []byte("SLOKs")
 	datastoreTacticsBucket                      = []byte("tactics")
 	datastoreSpeedTestSamplesBucket             = []byte("speedTestSamples")
@@ -47,6 +48,7 @@ var (
 	datastoreLastServerEntryFilterKey           = []byte("lastServerEntryFilter")
 	datastoreAffinityServerEntryIDKey           = []byte("affinityServerEntryID")
 	datastorePersistentStatTypeRemoteServerList = string(datastoreRemoteServerListStatsBucket)
+	datastorePersistentStatTypeFailedTunnel     = string(datastoreFailedTunnelStatsBucket)
 	datastoreServerEntryFetchGCThreshold        = 20
 
 	datastoreMutex    sync.RWMutex
@@ -961,6 +963,7 @@ var persistentStatStateReporting = []byte("1")
 
 var persistentStatTypes = []string{
 	datastorePersistentStatTypeRemoteServerList,
+	datastorePersistentStatTypeFailedTunnel,
 }
 
 // StorePersistentStat adds a new persistent stat record, which
@@ -970,17 +973,36 @@ var persistentStatTypes = []string{
 // The stat is a JSON byte array containing fields as
 // required by the Psiphon server API. It's assumed that the
 // JSON value contains enough unique information for the value to
-// function as a key in the key/value datastore. This assumption
-// is currently satisfied by the fields sessionId + tunnelNumber
-// for tunnel stats, and URL + ETag for remote server list stats.
-func StorePersistentStat(statType string, stat []byte) error {
+// function as a key in the key/value datastore.
+//
+// Only up to PersistentStatsMaxStoreRecords are stored. Once this
+// limit is reached, new records are discarded.
+func StorePersistentStat(config *Config, statType string, stat []byte) error {
 
 	if !common.Contains(persistentStatTypes, statType) {
 		return common.ContextError(fmt.Errorf("invalid persistent stat type: %s", statType))
 	}
 
+	maxStoreRecords := config.GetClientParameters().Int(parameters.PersistentStatsMaxStoreRecords)
+
 	err := datastoreUpdate(func(tx *datastoreTx) error {
 		bucket := tx.bucket([]byte(statType))
+
+		count := 0
+		cursor := bucket.cursor()
+		for key, _ := cursor.first(); key != nil; key, _ = cursor.next() {
+			count++
+		}
+		cursor.close()
+
+		// TODO: assuming newer metrics are more useful, replace oldest record
+		// instead of discarding?
+
+		if count >= maxStoreRecords {
+			// Silently discard.
+			return nil
+		}
+
 		err := bucket.put(stat, persistentStatStateUnreported)
 		return err
 	})
@@ -1023,18 +1045,21 @@ func CountUnreportedPersistentStats() int {
 	return unreported
 }
 
-// TakeOutUnreportedPersistentStats returns up to maxCount persistent
-// stats records that are in StateUnreported. The records are set to
-// StateReporting. If the records are successfully reported, clear them
+// TakeOutUnreportedPersistentStats returns persistent stats records that are
+// in StateUnreported. At least one record, if present, will be returned and
+// then additional records up to PersistentStatsMaxSendBytes. The records are
+// set to StateReporting. If the records are successfully reported, clear them
 // with ClearReportedPersistentStats. If the records are not successfully
 // reported, restore them with PutBackUnreportedPersistentStats.
-func TakeOutUnreportedPersistentStats(maxCount int) (map[string][][]byte, error) {
+func TakeOutUnreportedPersistentStats(config *Config) (map[string][][]byte, error) {
 
 	stats := make(map[string][][]byte)
 
+	maxSendBytes := config.GetClientParameters().Int(parameters.PersistentStatsMaxSendBytes)
+
 	err := datastoreUpdate(func(tx *datastoreTx) error {
 
-		count := 0
+		sendBytes := 0
 
 		for _, statType := range persistentStatTypes {
 
@@ -1042,18 +1067,15 @@ func TakeOutUnreportedPersistentStats(maxCount int) (map[string][][]byte, error)
 			cursor := bucket.cursor()
 			for key, value := cursor.first(); key != nil; key, value = cursor.next() {
 
-				if count >= maxCount {
-					break
-				}
-
 				// Perform a test JSON unmarshaling. In case of data corruption or a bug,
-				// skip the record.
+				// delete and skip the record.
 				var jsonData interface{}
 				err := json.Unmarshal(key, &jsonData)
 				if err != nil {
 					NoticeAlert(
 						"Invalid key in TakeOutUnreportedPersistentStats: %s: %s",
 						string(key), err)
+					bucket.delete(key)
 					continue
 				}
 
@@ -1067,8 +1089,13 @@ func TakeOutUnreportedPersistentStats(maxCount int) (map[string][][]byte, error)
 					}
 
 					stats[statType] = append(stats[statType], data)
-					count += 1
+
+					sendBytes += len(data)
+					if sendBytes >= maxSendBytes {
+						break
+					}
 				}
+
 			}
 			cursor.close()
 

psiphon/dataStore_bolt.go

@@ -97,6 +97,7 @@ func datastoreOpenDB(rootDataDirectory string) (*datastoreDB, error) {
 			datastoreUrlETagsBucket,
 			datastoreKeyValueBucket,
 			datastoreRemoteServerListStatsBucket,
+			datastoreFailedTunnelStatsBucket,
 			datastoreSLOKsBucket,
 			datastoreTacticsBucket,
 			datastoreSpeedTestSamplesBucket,

psiphon/remoteServerList.go

@@ -418,7 +418,7 @@ func downloadRemoteServerListFile(
 
 	NoticeRemoteServerListResourceDownloaded(sourceURL)
 
-	RecordRemoteServerListStat(sourceURL, responseETag)
+	_ = RecordRemoteServerListStat(config, sourceURL, responseETag)
 
 	return responseETag, nil
 }

psiphon/server/api.go

@@ -376,6 +376,25 @@ var statusRequestParams = append(
 		{"connected", isBooleanFlag, requestParamLogFlagAsBool}},
 	baseRequestParams...)
 
+var remoteServerListStatParams = []requestParamSpec{
+	{"session_id", isHexDigits, requestParamOptional},
+	{"propagation_channel_id", isHexDigits, requestParamOptional},
+	{"sponsor_id", isHexDigits, requestParamOptional},
+	{"client_version", isAnyString, requestParamOptional},
+	{"client_platform", isAnyString, requestParamOptional},
+	{"client_build_rev", isAnyString, requestParamOptional},
+	{"client_download_timestamp", isISO8601Date, 0},
+	{"url", isAnyString, 0},
+	{"etag", isAnyString, 0},
+}
+
+var failedTunnelStatParams = append(
+	[]requestParamSpec{
+		{"session_id", isHexDigits, 0},
+		{"client_failed_timestamp", isISO8601Date, 0},
+		{"tunnel_error", isAnyString, 0}},
+	baseRequestParams...)
+
 // statusAPIRequestHandler implements the "status" API request.
 // Clients make periodic status requests which deliver client-side
 // recorded data transfer and tunnel duration stats.
@@ -440,8 +459,8 @@ func statusAPIRequestHandler(
 		}
 	}
 
-	// Remote server list download stats
-	// Older clients may not submit this data
+	// Remote server list download stats. Older clients may not submit this data.
+	// Limitation: for "persistent" stats, geolocation is time-of-sending not time-of-recording.
 
 	if statusData["remote_server_list_stats"] != nil {
 
@@ -451,6 +470,12 @@ func statusAPIRequestHandler(
 		}
 		for _, remoteServerListStat := range remoteServerListStats {
 
+			err := validateRequestParams(support.Config, remoteServerListStat, remoteServerListStatParams)
+			if err != nil {
+				return nil, common.ContextError(err)
+			}
+
+			// remote_server_list defaults to using the common params from the outer statusRequestParams
 			remoteServerListFields := getRequestLogFields(
 				"remote_server_list",
 				geoIPData,
@@ -458,25 +483,38 @@ func statusAPIRequestHandler(
 				params,
 				statusRequestParams)
 
-			clientDownloadTimestamp, err := getStringRequestParam(remoteServerListStat, "client_download_timestamp")
-			if err != nil {
-				return nil, common.ContextError(err)
+			for name, value := range remoteServerListStat {
+				remoteServerListFields[name] = value
 			}
-			remoteServerListFields["client_download_timestamp"] = clientDownloadTimestamp
 
-			url, err := getStringRequestParam(remoteServerListStat, "url")
-			if err != nil {
-				return nil, common.ContextError(err)
-			}
-			remoteServerListFields["url"] = url
+			logQueue = append(logQueue, remoteServerListFields)
+		}
+	}
+
+	// Failed tunnel stats. Older clients may not submit this data.
+	// Limitation: for "persistent" stats, geolocation is time-of-sending not time-of-recording.
+
+	if statusData["failed_tunnel_stats"] != nil {
+
+		failedTunnelStats, err := getJSONObjectArrayRequestParam(statusData, "failed_tunnel_stats")
+		if err != nil {
+			return nil, common.ContextError(err)
+		}
+		for _, failedTunnelStat := range failedTunnelStats {
 
-			etag, err := getStringRequestParam(remoteServerListStat, "etag")
+			err := validateRequestParams(support.Config, failedTunnelStat, failedTunnelStatParams)
 			if err != nil {
 				return nil, common.ContextError(err)
 			}
-			remoteServerListFields["etag"] = etag
 
-			logQueue = append(logQueue, remoteServerListFields)
+			failedTunnelFields := getRequestLogFields(
+				"failed_tunnel",
+				geoIPData,
+				authorizedAccessTypes,
+				failedTunnelStat,
+				failedTunnelStatParams)
+
+			logQueue = append(logQueue, failedTunnelFields)
 		}
 	}
 

psiphon/serverApi.go

@@ -34,7 +34,6 @@ import (
 	"net/url"
 	"strconv"
 	"strings"
-	"sync/atomic"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/parameters"
@@ -50,11 +49,6 @@ import (
 // offer the Psiphon API through a web service; newer servers offer the Psiphon
 // API through SSH requests made directly through the tunnel's SSH client.
 type ServerContext struct {
-	// Note: 64-bit ints used with atomic operations are placed
-	// at the start of struct to ensure 64-bit alignment.
-	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
-	tunnelNumber             int64
-	sessionId                string
 	tunnel                   *Tunnel
 	psiphonHttpsClient       *http.Client
 	statsRegexps             *transferstats.Regexps
@@ -64,13 +58,6 @@ type ServerContext struct {
 	paddingPRNG              *prng.PRNG
 }
 
-// nextTunnelNumber is a monotonically increasing number assigned to each
-// successive tunnel connection. The sessionId and tunnelNumber together
-// form a globally unique identifier for tunnels, which is used for
-// stats. Note that the number is increasing but not necessarily
-// consecutive for each active tunnel in session.
-var nextTunnelNumber int64
-
 // MakeSessionId creates a new session ID. The same session ID is used across
 // multi-tunnel controller runs, where each tunnel has its own ServerContext
 // instance.
@@ -104,8 +91,6 @@ func NewServerContext(tunnel *Tunnel) (*ServerContext, error) {
 	}
 
 	serverContext := &ServerContext{
-		sessionId:          tunnel.sessionId,
-		tunnelNumber:       atomic.AddInt64(&nextTunnelNumber, 1),
 		tunnel:             tunnel,
 		psiphonHttpsClient: psiphonHttpsClient,
 		paddingPRNG:        prng.NewPRNGWithSeed(tunnel.dialParams.APIRequestPaddingSeed),
@@ -386,7 +371,7 @@ func (serverContext *ServerContext) DoStatusRequest(tunnel *Tunnel) error {
 	// payload for future attempt, in all failure cases.
 
 	statusPayload, statusPayloadInfo, err := makeStatusRequestPayload(
-		serverContext.tunnel.config.clientParameters,
+		serverContext.tunnel.config,
 		tunnel.dialParams.ServerEntry.IpAddress)
 	if err != nil {
 		return common.ContextError(err)
@@ -467,15 +452,13 @@ type statusRequestPayloadInfo struct {
 }
 
 func makeStatusRequestPayload(
-	clientParameters *parameters.ClientParameters,
+	config *Config,
 	serverId string) ([]byte, *statusRequestPayloadInfo, error) {
 
 	transferStats := transferstats.TakeOutStatsForServer(serverId)
 	hostBytes := transferStats.GetStatsForStatusRequest()
 
-	maxCount := clientParameters.Get().Int(parameters.PsiphonAPIPersistentStatsMaxCount)
-
-	persistentStats, err := TakeOutUnreportedPersistentStats(maxCount)
+	persistentStats, err := TakeOutUnreportedPersistentStats(config)
 	if err != nil {
 		NoticeAlert(
 			"TakeOutUnreportedPersistentStats failed: %s", common.ContextError(err))
@@ -502,6 +485,7 @@ func makeStatusRequestPayload(
 
 	persistentStatPayloadNames := make(map[string]string)
 	persistentStatPayloadNames[datastorePersistentStatTypeRemoteServerList] = "remote_server_list_stats"
+	persistentStatPayloadNames[datastorePersistentStatTypeFailedTunnel] = "failed_tunnels"
 
 	for statType, stats := range persistentStats {
 
@@ -554,15 +538,8 @@ func confirmStatusRequestPayload(payloadInfo *statusRequestPayloadInfo) {
 // records are stored in the persistent datastore and reported
 // via subsequent status requests sent to any Psiphon server.
 //
-// Note that common event field values may change between the
-// stat recording and reporting include client geo data,
-// propagation channel, sponsor ID, client version. These are not
-// stored in the datastore (client region, in particular, since
-// that would create an on-disk record of user location).
-// TODO: the server could encrypt, with a nonce and key unknown to
-// the client, a blob containing this data; return it in the
-// handshake response; and the client could store and later report
-// this blob with its tunnel stats records.
+// Note that some common event field values may change between the
+// stat recording and reporting, including client geolocation.
 //
 // Multiple "status" requests may be in flight at once (due
 // to multi-tunnel, asynchronous final status retry, and
@@ -575,25 +552,59 @@ func confirmStatusRequestPayload(payloadInfo *statusRequestPayloadInfo) {
 // processes a status request but the client fails to receive
 // the response.
 func RecordRemoteServerListStat(
-	url, etag string) error {
+	config *Config, url, etag string) error {
 
-	remoteServerListStat := struct {
-		ClientDownloadTimestamp string `json:"client_download_timestamp"`
-		URL                     string `json:"url"`
-		ETag                    string `json:"etag"`
-	}{
-		common.TruncateTimestampToHour(common.GetCurrentTimestamp()),
-		url,
-		etag,
+	if !config.GetClientParameters().Bool(
+		parameters.RecordRemoteServerListPersistentStats) {
+		return nil
 	}
 
-	remoteServerListStatJson, err := json.Marshal(remoteServerListStat)
+	params := make(common.APIParameters)
+
+	params["session_id"] = config.SessionID
+	params["propagation_channel_id"] = config.PropagationChannelId
+	params["sponsor_id"] = config.GetSponsorID()
+	params["client_version"] = config.ClientVersion
+	params["client_platform"] = config.ClientPlatform
+	params["client_build_rev"] = common.GetBuildInfo().BuildRev
+
+	params["client_download_timestamp"] = common.TruncateTimestampToHour(common.GetCurrentTimestamp())
+	params["url"] = url
+	params["etag"] = etag
+
+	remoteServerListStatJson, err := json.Marshal(params)
+	if err != nil {
+		return common.ContextError(err)
+	}
+
+	return StorePersistentStat(
+		config, datastorePersistentStatTypeRemoteServerList, remoteServerListStatJson)
+}
+
+// RecordFailedTunnelStat records metrics for a failed tunnel dial, including
+// dial parameters and error condition (tunnelErr).
+//
+// This uses the same reporting facility, with the same caveats, as
+// RecordRemoteServerListStat.
+func RecordFailedTunnelStat(
+	config *Config, dialParams *DialParameters, tunnelErr error) error {
+
+	if !config.GetClientParameters().Bool(
+		parameters.RecordFailedTunnelPersistentStats) {
+		return nil
+	}
+
+	params := getBaseAPIParameters(config, dialParams)
+	params["client_failed_timestamp"] = common.TruncateTimestampToHour(common.GetCurrentTimestamp())
+	params["tunnel_error"] = tunnelErr.Error()
+
+	failedTunnelStatJson, err := json.Marshal(params)
 	if err != nil {
 		return common.ContextError(err)
 	}
 
 	return StorePersistentStat(
-		datastorePersistentStatTypeRemoteServerList, remoteServerListStatJson)
+		config, datastorePersistentStatTypeFailedTunnel, failedTunnelStatJson)
 }
 
 // doGetRequest makes a tunneled HTTPS request and returns the response body.
@@ -667,7 +678,6 @@ func (serverContext *ServerContext) getBaseAPIParameters() common.APIParameters
 
 	params := getBaseAPIParameters(
 		serverContext.tunnel.config,
-		serverContext.sessionId,
 		serverContext.tunnel.dialParams)
 
 	// Add a random amount of padding to defend against API call traffic size
@@ -698,13 +708,12 @@ func (serverContext *ServerContext) getBaseAPIParameters() common.APIParameters
 // for metrics.
 func getBaseAPIParameters(
 	config *Config,
-	sessionID string,
 	dialParams *DialParameters) common.APIParameters {
 
 	params := make(common.APIParameters)
 
-	params["session_id"] = sessionID
-	params["client_session_id"] = sessionID
+	params["session_id"] = config.SessionID
+	params["client_session_id"] = config.SessionID
 	params["server_secret"] = dialParams.ServerEntry.WebServerSecret
 	params["propagation_channel_id"] = config.PropagationChannelId
 	params["sponsor_id"] = config.GetSponsorID()

psiphon/tunnel.go

@@ -87,7 +87,6 @@ type Tunnel struct {
 	isActivated                bool
 	isDiscarded                bool
 	isClosed                   bool
-	sessionId                  string
 	dialParams                 *DialParameters
 	serverContext              *ServerContext
 	conn                       *common.ActivityMonitoredConn
@@ -126,14 +125,12 @@ type Tunnel struct {
 func ConnectTunnel(
 	ctx context.Context,
 	config *Config,
-	sessionId string,
 	adjustedEstablishStartTime monotime.Time,
 	dialParams *DialParameters) (*Tunnel, error) {
 
 	// Build transport layers and establish SSH connection. Note that
 	// dialConn and monitoredConn are the same network connection.
-	dialResult, err := dialTunnel(
-		ctx, config, sessionId, dialParams)
+	dialResult, err := dialTunnel(ctx, config, dialParams)
 	if err != nil {
 		return nil, common.ContextError(err)
 	}
@@ -142,7 +139,6 @@ func ConnectTunnel(
 	return &Tunnel{
 		mutex:             new(sync.Mutex),
 		config:            config,
-		sessionId:         sessionId,
 		dialParams:        dialParams,
 		conn:              dialResult.monitoredConn,
 		sshClient:         dialResult.sshClient,
@@ -158,8 +154,7 @@ func ConnectTunnel(
 // request and starting operateTunnel, the worker that monitors the tunnel
 // and handles periodic management.
 func (tunnel *Tunnel) Activate(
-	ctx context.Context,
-	tunnelOwner TunnelOwner) error {
+	ctx context.Context, tunnelOwner TunnelOwner) (retErr error) {
 
 	// Ensure that, unless the context is cancelled, any replayed dial
 	// parameters are cleared, no longer to be retried, if the tunnel fails to
@@ -168,6 +163,7 @@ func (tunnel *Tunnel) Activate(
 	defer func() {
 		if !activationSucceeded && ctx.Err() == nil {
 			tunnel.dialParams.Failed()
+			_ = RecordFailedTunnelStat(tunnel.config, tunnel.dialParams, retErr)
 		}
 	}()
 
@@ -525,8 +521,7 @@ type dialResult struct {
 func dialTunnel(
 	ctx context.Context,
 	config *Config,
-	sessionId string,
-	dialParams *DialParameters) (*dialResult, error) {
+	dialParams *DialParameters) (_ *dialResult, retErr error) {
 
 	// Return immediately when overall context is canceled or timed-out. This
 	// avoids notice noise.
@@ -558,6 +553,7 @@ func dialTunnel(
 	defer func() {
 		if !dialSucceeded && baseCtx.Err() == nil {
 			dialParams.Failed()
+			_ = RecordFailedTunnelStat(config, dialParams, retErr)
 		}
 	}()
 
@@ -718,7 +714,7 @@ func dialTunnel(
 	}
 
 	sshPasswordPayload := &protocol.SSHPasswordPayload{
-		SessionId:          sessionId,
+		SessionId:          config.SessionID,
 		SshPassword:        dialParams.ServerEntry.SshPassword,
 		ClientCapabilities: []string{protocol.CLIENT_CAPABILITY_SERVER_REQUESTS},
 	}