Send a telemetry event on exit #541
Conversation
| @@ -18,39 +18,42 @@ describe("telemetry", () => { | |||
| setGlobalDispatcher(globalDispatcher); | |||
| }); | |||
|
|
|||
| const noopEffects = {env: {}, logger: new MockLogger(), getPersistentId: async () => randomUUID()}; | |||
| const processMock = (mock: any) => Object.assign(Object.create(process), mock); | |||
There was a problem hiding this comment.
This was easy enough to write, but would it be worth it to bring in something like https://sinonjs.org for this stubbing and mocking?
| if (!this._config) { | ||
| this._config = readFile(file, "utf8") | ||
| .then(JSON.parse) | ||
| .catch(() => ({})); | ||
| } |
There was a problem hiding this comment.
Something that we ran into in deploys is that treating all errors the same here can be confusing for the user. If the file doesn't exist returning {} is useful, but if there is a syntax error (like JSON.parse might throw), I think we should not treat it as a empty config.
One example I can think of: if I edit my config and make a syntax error (say, deleting a comma), then the next time this function runs it will delete my authorization information.
I think we should consider invalid syntax as an invalid config. Maybe that would mean returning null here? Or maybe even an error should be thrown to the user?
There was a problem hiding this comment.
Ok, I think then in the case of JSON syntax error we just return null here then since I don't really want telemetry to be responsible for telling the user to fix a malformed configuration error. So if other code is using that same file, we'll hope/trust/cross-fingers that it'll raise a general issue about the file being malformed?
There was a problem hiding this comment.
Egh, maybe we should consolidate ~/.observablehq file handling code though and just error. Because there are race condition dragons here.
|
Awaiting a new record() at the end, even a stub-ish one, returns a Promise.all to the pending queue. So, it should get you what you want. |
visnup
force-pushed
the
visnup/telemetry-on-exit
branch
from
e5ddb38 to
c7dd255
Compare
| assert.throws(() => { | ||
| new Telemetry({ | ||
| ...noopEffects, | ||
| process: processMock({env: {OBSERVABLE_TELEMETRY_ORIGIN: "☃️"}}) |
There was a problem hiding this comment.
if I recall correctly, Rails used to force some http guarantees by submitting forms with ?utf8=☃ in a request param. it got changed pretty quickly to ?utf8=✓ to confuse/scare people less often. I dunno if that's still around anymore or how I'd even find it.
There was a problem hiding this comment.
or maybe it was a heuristic to tell if a proxy or server in the middle of the request chain preserved it correctly all the way through?
There was a problem hiding this comment.
now to figure out if my memory of it ever being a snowman value is correct or I'm combining two different memories…
There was a problem hiding this comment.
thank you GitHub rails/rails@1cbe917
|
Going to work on consolidating ~/.observablehq handling code to handle any race conditions today. Will follow up with another PR… I also have a small change to add a the session index / id / handle thing, but realized I'd need to update the incoming endpoint to handle it first. |
Handles SIGINT, SIGHUP, and SIGTERM and sends an exiting telemetry event along with flushing its queue. Waits up to 1s to finish then exits no matter what. Doesn't install any signal listeners unless
Telemetry.recordis actually called, so nothing is sent on unwatched commands likehelp.Sneaks in a few more data to track: node version and build page count.
Adds more test coverage of
getPersistentIdby moving the effect abstraction down toreadFileandwriteFile.I'm a little wary of calling effects.process.exit inside of the signal handler since whatever's passed in may not actually exit which would hang the process. Maybe typescript will help since it'll enforce whatever is passed in for it has a
neverreturn type?