Skip to content

Commit

Permalink
fix(app-shell, app-shell-odd): reduce port block analytics reporting …
Browse files Browse the repository at this point in the history 
…frequency

Instead of reporting every port block event, let's just report one event per session of the desktop
app/odd.
  • Loading branch information
@mjhuff
mjhuff committed Feb 26, 2024
1 parent 10c0e77 commit ba5ba22
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 12 deletions.
24 changes: 18 additions & 6 deletions app-shell-odd/src/notify.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import type { Action, Dispatch } from './types'
// However, redundant subscriptions are permitted and result in the broker sending the retained message for that topic.
// To mitigate redundant connections, the connection manager eagerly adds the host, removing the host if the connection fails.

const FAILURE_STATUSES = {

Check warning on line 18 in app-shell-odd/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell-odd/src/notify.ts#L18 

Added line #L18 was not covered by tests
ECONNREFUSED: 'ECONNREFUSED',
ECONNFAILED: 'ECONNFAILED',
} as const

interface ConnectionStore {
[hostname: string]: {
client: mqtt.MqttClient | null
Expand Down Expand Up @@ -72,6 +77,7 @@ export function registerNotify(
}

const CHECK_CONNECTION_INTERVAL = 500
let hasReportedAPortBlockEvent = false

Check warning on line 80 in app-shell-odd/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell-odd/src/notify.ts#L80 

Added line #L80 was not covered by tests

interface NotifyParams {
browserWindow: BrowserWindow
Expand Down Expand Up @@ -101,9 +107,15 @@ function subscribe(notifyParams: NotifyParams): Promise<void> {
log.warn(
`Failed to connect to ${hostname} - ${error.name}: ${error.message} `
)
const failureMessage = error.message.includes('ECONNREFUSED')
? 'ECONNREFUSED'
: 'ECONNFAILED'

let failureMessage: string = FAILURE_STATUSES.ECONNFAILED

Check warning on line 111 in app-shell-odd/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell-odd/src/notify.ts#L111 

Added line #L111 was not covered by tests
if (
error.message.includes(FAILURE_STATUSES.ECONNREFUSED) &&
!hasReportedAPortBlockEvent
) {
failureMessage = FAILURE_STATUSES.ECONNREFUSED
hasReportedAPortBlockEvent = true

Check warning on line 117 in app-shell-odd/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell-odd/src/notify.ts#L116-L117 

Added lines #L116 - L117 were not covered by tests
}

sendToBrowserDeserialized({
browserWindow,
Expand All @@ -128,7 +140,7 @@ function subscribe(notifyParams: NotifyParams): Promise<void> {
browserWindow,
hostname,
topic,
message: 'ECONNFAILED',
message: FAILURE_STATUSES.ECONNFAILED,
})
handleDecrementSubscriptionCount(hostname, topic)
})
Expand All @@ -141,7 +153,7 @@ function subscribe(notifyParams: NotifyParams): Promise<void> {
browserWindow,
hostname,
topic,
message: 'ECONNFAILED',
message: FAILURE_STATUSES.ECONNFAILED,
})
handleDecrementSubscriptionCount(hostname, topic)
} else {
Expand Down Expand Up @@ -312,7 +324,7 @@ function establishListeners({
browserWindow,
hostname,
topic,
message: 'ECONNFAILED',
message: FAILURE_STATUSES.ECONNFAILED,
})
client.end()
})
Expand Down
24 changes: 18 additions & 6 deletions app-shell/src/notify.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ import type { Action, Dispatch } from './types'
// However, redundant subscriptions are permitted and result in the broker sending the retained message for that topic.
// To mitigate redundant connections, the connection manager eagerly adds the host, removing the host if the connection fails.

const FAILURE_STATUSES = {

Check warning on line 16 in app-shell/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/notify.ts#L16 

Added line #L16 was not covered by tests
ECONNREFUSED: 'ECONNREFUSED',
ECONNFAILED: 'ECONNFAILED',
} as const

interface ConnectionStore {
[hostname: string]: {
client: mqtt.MqttClient | null
Expand Down Expand Up @@ -68,6 +73,7 @@ export function registerNotify(
}

const CHECK_CONNECTION_INTERVAL = 500
let hasReportedAPortBlockEvent = false

Check warning on line 76 in app-shell/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/notify.ts#L76 

Added line #L76 was not covered by tests

interface NotifyParams {
browserWindow: BrowserWindow
Expand Down Expand Up @@ -97,9 +103,15 @@ function subscribe(notifyParams: NotifyParams): Promise<void> {
log.warn(
`Failed to connect to ${hostname} - ${error.name}: ${error.message} `
)
const failureMessage = error.message.includes('ECONNREFUSED')
? 'ECONNREFUSED'
: 'ECONNFAILED'

let failureMessage: string = FAILURE_STATUSES.ECONNFAILED

Check warning on line 107 in app-shell/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/notify.ts#L107 

Added line #L107 was not covered by tests
if (
error.message.includes(FAILURE_STATUSES.ECONNREFUSED) &&
!hasReportedAPortBlockEvent
) {
failureMessage = FAILURE_STATUSES.ECONNREFUSED
hasReportedAPortBlockEvent = true

Check warning on line 113 in app-shell/src/notify.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/notify.ts#L112-L113 

Added lines #L112 - L113 were not covered by tests
}

sendToBrowserDeserialized({
browserWindow,
Expand All @@ -124,7 +136,7 @@ function subscribe(notifyParams: NotifyParams): Promise<void> {
browserWindow,
hostname,
topic,
message: 'ECONNFAILED',
message: FAILURE_STATUSES.ECONNFAILED,
})
handleDecrementSubscriptionCount(hostname, topic)
})
Expand All @@ -137,7 +149,7 @@ function subscribe(notifyParams: NotifyParams): Promise<void> {
browserWindow,
hostname,
topic,
message: 'ECONNFAILED',
message: FAILURE_STATUSES.ECONNFAILED,
})
handleDecrementSubscriptionCount(hostname, topic)
} else {
Expand Down Expand Up @@ -308,7 +320,7 @@ function establishListeners({
browserWindow,
hostname,
topic,
message: 'ECONNFAILED',
message: FAILURE_STATUSES.ECONNFAILED,
})
client.end()
})
Expand Down

0 comments on commit ba5ba22

Please sign in to comment.