Fix error handling for metrics #1
Conversation
src/shadowbox/server/ip_util.ts
Outdated
| }); | ||
| }); | ||
| }); | ||
| export class CachedIpLocationService implements IpLocationService { |
There was a problem hiding this comment.
Servers could stay up for weeks - let's file an issue to make this cache:
- bounded in size
- an expiring cache
There was a problem hiding this comment.
Added the comment. We should make it LRU.
src/shadowbox/server/ip_util.ts
Outdated
| fulfill('ZZ'); | ||
| } | ||
| } catch (e) { | ||
| reject(new Error(`Error loading country from reponse: ${e}`)); |
There was a problem hiding this comment.
Seems like there's a chance this could log the IP address - I'd feel better if we weren't logging the error verbatim.
There was a problem hiding this comment.
Removed the cause error
| countryPromises.push(lookupCountry(ip)); | ||
| const countryPromise = ipLocationService.countryForIp(ip).catch((e) => { | ||
| console.warn('Failed countryForIp call: ', e); | ||
| return 'ERROR'; |
There was a problem hiding this comment.
It's hard to figure out what the outcome of this is - are you filtering the unsuccessful lookups?
There was a problem hiding this comment.
We save the country as ERROR on unsuccessful lookups
There was a problem hiding this comment.
OK...and that makes it right through to BigQuery?
I think you should remove the console.warn. Seems like logging on first failure is sufficient.
There was a problem hiding this comment.
This is the only place we log. The IpLocationService doesn't log anything.
I consider a good approach to log errors only where you handle them (like here). This about the duplicate logging.
src/server_manager/web_app/main.ts
Outdated
| if (!queryParam) { | ||
| return null; | ||
| } | ||
| if (typeof(queryParam) === 'string') { |
There was a problem hiding this comment.
IIRC, testing for strings is a minefield in JavaScript. Array.isArray might be safer.
There was a problem hiding this comment.
Changed to Array.isArray
|
@fortuna One more request - and please consider a squash commit when merging. |
* chore: upgrade to lts/hydrogen * update github workflows * fix attempt #1 * ssl, restify upgrade * more openssl options * ah thanks sander, using middleware2 * fix ci * reset package-lock.json * revert middleware2 * fix optional chaining * okay cool tests run, but fail * partially update the node image (i can't find the other one) * Update test.action.sh * Update build.action.sh * Update test.action.sh * Update build.action.sh * Update package.json * Update webpack.config.js * Update webpack.config.js * Update build.action.sh * Update build.action.sh * Update package.json * Update README.md
cc @trevj @sandrigo FYI