Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: different countly keys for kubo and webui.ipfs.io deployments #2081

Merged
merged 10 commits into from
Jan 27, 2023
Merged

feat: different countly keys for kubo and webui.ipfs.io deployments #2081

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
85c8f3a
feat: different countly keys for kubo and webui.ipfs.io deployments
SgtPooki Jan 16, 2023
c2fbccc
fix: check origin for deploymentEnv first
SgtPooki Jan 25, 2023
f9d9c52
fix: dont typecheck everything yet..
SgtPooki Jan 25, 2023
8465c9d
Merge branch 'main' into 2078-ensure-we-have-a-way-to-differentiate-b…
SgtPooki Jan 27, 2023
d15e1b7
fix: use countly.ipfs.tech url
SgtPooki Jan 27, 2023
d9d18b8
fix: analytics tests succeed
SgtPooki Jan 27, 2023
b734ab5
fix(env.js): use regex for string matching
SgtPooki Jan 27, 2023
e1f0296
chore(env.js): add tests
SgtPooki Jan 27, 2023
364dcba
chore(env.test.js): reset globalThis.fetch completely when done
SgtPooki Jan 27, 2023
827c13a
chore: address pr comments
SgtPooki Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/bundles/analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { ACTIONS as FILES } from './files/consts'
import { ACTIONS as CONIFG } from './config-save'
import { ACTIONS as INIT } from './ipfs-provider'
import { ACTIONS as EXP } from './experiments'
import { getDeploymentEnv } from '../env'

/**
* @typedef {import('./ipfs-provider').Init} Init
Expand Down Expand Up @@ -98,13 +99,22 @@ const ASYNC_ACTIONS_TO_RECORD = [

const COUNTLY_KEY_WEBUI = '8fa213e6049bff23b08e5f5fbac89e7c27397612'
const COUNTLY_KEY_WEBUI_TEST = '700fd825c3b257e021bd9dbc6cbf044d33477531'
const COUNTLY_KEY_WEBUI_KUBO = 'c4524cc93fed92a5838d4ea27c5a65526b4e7558'

function pickAppKey () {
/**
* @see https://github.com/ipfs/ipfs-webui/issues/2078
* @returns {Promise<string>}
*/
async function pickAppKey () {
const isProd = process.env.NODE_ENV === 'production'

if (root.ipfsDesktop && root.ipfsDesktop.countlyAppKey) {
return root.ipfsDesktop.countlyAppKey
} else {
const env = await getDeploymentEnv()
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
if (env === 'kubo') {
return COUNTLY_KEY_WEBUI_KUBO
}
return isProd ? COUNTLY_KEY_WEBUI : COUNTLY_KEY_WEBUI_TEST
}
}
Expand Down Expand Up @@ -274,8 +284,7 @@ const actions = {
}

const createAnalyticsBundle = ({
countlyUrl = 'https://countly.ipfs.io',
countlyAppKey = pickAppKey(),
countlyUrl = 'https://countly.ipfs.tech',
appVersion = process.env.REACT_APP_VERSION,
// @ts-ignore - declared but never used
appGitRevision = process.env.REACT_APP_GIT_REV,
Expand Down Expand Up @@ -308,7 +317,10 @@ const createAnalyticsBundle = ({

Countly.require_consent = true
Countly.url = countlyUrl
Countly.app_key = countlyAppKey
const countlyAppKeyPromise = pickAppKey()
countlyAppKeyPromise.then((appKey) => {
Countly.app_key = appKey
})
Comment on lines +320 to +323
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const countlyAppKeyPromise = pickAppKey()
countlyAppKeyPromise.then((appKey) => {
Countly.app_key = appKey
})
Countly.app_key = await pickAppKey()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎 this actually fails some existing tests and was how I had the code originally

Countly.app_version = appVersion
Countly.debug = debug

Expand Down Expand Up @@ -348,6 +360,7 @@ const createAnalyticsBundle = ({

// Fix for storybook error 'Countly.init is not a function'
if (typeof Countly.init === 'function') {
await countlyAppKeyPromise
Countly.init()
}
},
Expand Down
31 changes: 31 additions & 0 deletions src/env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@

/**
* @see https://github.com/ipfs/ipfs-webui/issues/2078#issuecomment-1384444232
* @see https://github.com/ipfs/ipfs-webui/issues/2078#issuecomment-1384605253
* @returns {Promise<'local'|'kubo'|'webui.ipfs'>}
*/

const webuiRegex = /(?:webui-ipfs|webui\.ipfs)/
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
const localhostRegex = /localhost/
export async function getDeploymentEnv () {
const { origin } = globalThis.location
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
if (webuiRegex.test(origin)) {
if (localhostRegex.test(origin)) {
return 'local'
}
return 'webui.ipfs'
}
try {
const response = await fetch('/webui', { method: 'HEAD' })
if (response.redirected) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:
is this kubo check necessary? if not local or webui.ipfs.io it would be kubo correct?
is this check just a safeguard for future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it is required as we can't really guarantee that it's kubo unless it meets the expectations of kubo. We should assume it's typical webui unless we can guarantee it's kubo. I added some tests to express this more clearly, but as lidel mentioned at #2078 (comment) it's susceptible to silent failures upon kubo's changing of behavior.

Copy link
Member Author

@SgtPooki SgtPooki Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that question was something i was wondering myself after addressing lidel’s latest comment. but i’d rather have some webui metrics for kubo under webui (when/if they change behavior) than non-kubo webui metrics listed for kubo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, tests help too. LGTM

/**
* @see https://github.com/ipfs/kubo/blob/f54b2bcf6061219f7f481e8660e6707ae3bc3c38/cmd/ipfs/daemon.go#L677
* @see https://github.com/ipfs/kubo/blob/15093a00116ecf6b550023155f31a33f4bba6403/core/corehttp/webui.go#L57
*/
return 'kubo'
}
return 'local'
} catch {
return 'webui.ipfs'
}
}
1 change: 1 addition & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,6 @@
"src/lib/count-dirs.js",
"src/lib/sort.js",
"src/lib/files.js",
"src/env.js"
]
}