-
Notifications
You must be signed in to change notification settings - Fork 486
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.
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
feat: different countly keys for kubo and webui.ipfs.io deployments #2081
feat: different countly keys for kubo and webui.ipfs.io deployments #2081
Conversation
Need to update this to reflect lidel's recommendation in #2078 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self review
src/env.js
Outdated
export async function getDeploymentEnv () { | ||
const origin = globalThis.location.origin | ||
if (origin.includes('webui-ipfs') || origin.includes('webui.ipfs')) { | ||
if (origin.includes('localhost')) { | ||
return 'local' | ||
} | ||
return 'webui.ipfs' | ||
} | ||
try { | ||
const response = await fetch('/webui', { method: 'HEAD' }) | ||
if (response.redirected) { | ||
/** | ||
* @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' | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidel how does this look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm in principle I'll let @lidel approve.
…etween-webuiipfsio-and-webui-from-kubo
src/env.js
Outdated
} | ||
try { | ||
const response = await fetch('/webui', { method: 'HEAD' }) | ||
if (response.redirected) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great, some nits
const countlyAppKeyPromise = pickAppKey() | ||
countlyAppKeyPromise.then((appKey) => { | ||
Countly.app_key = appKey | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const countlyAppKeyPromise = pickAppKey() | |
countlyAppKeyPromise.then((appKey) => { | |
Countly.app_key = appKey | |
}) | |
Countly.app_key = await pickAppKey() |
There was a problem hiding this comment.
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
if (webuiRegex.test(origin)) { | ||
return localhostRegex.test(origin) ? 'local' : 'webui.ipfs' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not move this in the try block too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can regex.test actually throw? The only way I can think of is if the object passed to it throws on toString:
var f = /test/
var o = { toString: () => { throw new Error('test') }}
f.test(o)
return 'kubo' | ||
} | ||
} catch { | ||
// dont throw from this method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a debug log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh.. i don't see what value we would get from it. This is an expected throw in the majority of usecases (maybe not the majority after we see the metrics)
testOriginAndEnv('https://webui.ipfs.io', () => {}, 'webui.ipfs', 0) | ||
testOriginAndEnv('https://webui-ipfs-io.ipns.dweb.link/', () => {}, 'webui.ipfs', 0) | ||
testOriginAndEnv('https://webui.ipfs.tech', () => {}, 'webui.ipfs', 0) | ||
testOriginAndEnv('https://some-random-url', () => { throw new Error('no match on origin') }, 'webui.ipfs', 1) | ||
testOriginAndEnv('https://some-random-url', () => ({ redirected: false, url: 'https://some-random-url/webui' }), 'webui.ipfs', 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a small improvement could be to use named args (destructured args) where we can have default fetchImpl = () => {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, I thought about that, but the parameter list is cleaner when writing the tests. I usually default to arg objects as it makes changes easy.. but I'm hoping (save the link to this comment =P) that we won't need to adjust this much.
## [2.22.0](v2.21.0...v2.22.0) (2023-01-27) CID `bafybeifeqt7mvxaniphyu2i3qhovjaf3sayooxbh5enfdqtiehxjv2ldte` --- ### Features * different countly keys for kubo and webui.ipfs.io deployments ([#2081](#2081)) ([2e766fa](2e766fa)) ### Bug Fixes * import status window UX ([#2075](#2075)) ([4104cc6](4104cc6)) * webui update metrics to opt-out by default (part of 2074) ([#2084](#2084)) ([cac7663](cac7663)) ### Trivial Changes * pull new translations ([#2076](#2076)) ([ad9e4ff](ad9e4ff))
🎉 This PR is included in version 2.22.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
fix #2078