Initial application logging setup #1102

carolinemodic · 2021-10-22T01:22:03Z

Initial framework for logging from client side applications in addition to a sample implementation in election-manager. This will likely change over time as I solidify things.

Its possible it might make sense for /logging to be its own repo so it can also be used in places like kiosk-browser.... but that would add friction whenever you need to add a new log so not sure the tradeoff there.

#1095
#1096
#1097

eventualbuddha · 2021-10-22T03:17:33Z

libs/logging/src/logger.ts

+      disposition = LogDispositionStandardTypes.NotApplicable,
+      ...additionalData
+    } = logData;
+    const logLine = {


This doesn't have a type annotation but probably should.

eventualbuddha · 2021-10-22T03:19:34Z

libs/logging/src/logger.ts

+      ...additionalData,
+    };
+    if (this.isClientSide) {
+      await fetch('/log', {


If we're posting an HTTP service then we should include a time stamp (and probably an incrementing counter) to ensure the server can properly reconstruct the order (and maybe identify any missing logs?).

eventualbuddha · 2021-10-22T03:20:14Z

libs/logging/src/types.ts

+  NotApplicable = 'na',
+}
+
+export type LoggingUserRole = CardDataTypes | 'vx-admin' | 'system';


Should these be from the LogSource enum with the same values? Should this be an enum when all the other similar types are too?

LogSource is the application making the log, this is the user causing the log to happen, vx-admin here is meant to represent a votingworks employee making a change, vs it referring to the name of election manager in LogSource but I see that that is confusing so I'll come up with a better name here.

eventualbuddha · 2021-10-22T03:22:14Z

libs/logging/tsconfig.test.json

@@ -0,0 +1,16 @@
+{
+  "extends": "./tsconfig.json",


I think the better split here is actually tsconfig.build.json (for building, which excludes tests) and tsconfig.json (for IDE/type checking).

oh I see, I was just copying how utils is set up.

eventualbuddha · 2021-10-22T03:23:45Z

apps/election-manager/prodserver/setupProxy.js

+  app.use(express.json({ limit: '5mb', type: 'application/json' }));
+  app.post('/log', (req, res) => {
+    console.log(req.body)
+    res.json({success: true})


nit: formatting

eventualbuddha · 2021-10-22T03:27:57Z

vxsuite.code-workspace

@@ -39,6 +39,9 @@
    {
      "path": "libs/ui"
    },
+    {
+      "path": "libs/logging"


Don't forget to add this library to the CircleCI config too.

eventualbuddha · 2021-10-22T03:32:20Z

libs/logging/src/logEventIDs.ts

+export enum LogEventId {
+  ElectionConfigured = 'election-configured',
+  ElectionUnconfigured = 'election-unconfigured',
+  MachineBoot = 'machine-booted',


This feels more like "application started" than actual machine boot.

This log is defined here for completeness its not going to be logged from the application it will be a reprocessed version of the actual machine boot log from the syslog.

eventualbuddha · 2021-10-22T03:34:58Z

libs/logging/src/logger.test.ts

+  const calls = fetchMock.calls('/log');
+  expect(calls).toHaveLength(1);
+  assert(calls[0] && calls[0][1]);
+  expect(calls[0][1]).toMatchObject(


Avoid the previous assert:

Suggested change

expect(calls[0][1]).toMatchObject(

expect(calls[0]?.[1]).toMatchObject(

lol for some reason I keep thinking when I'm coding that avoiding using "!" also means we can't use "?" which is obviously not true. Thanks for catching.

benadida

wondering about architecture, still thinking that a kiosk-browser API would introduce fewer moving parts than a service.

benadida · 2021-10-25T15:56:15Z

apps/election-manager/prodserver/setupProxy.js

+  app.post('/log', (req, res) => {
+    console.log(JSON.stringify(req.body))
+    res.json({ success: true })
+  })


I thought design doc had refocused on using a kiosk-browser call instead of an HTTP call?

I think I would prefer a kiosk-browser call instead of an additional service running, but maybe I'm missing a key reason for this?

It's not really an additional service (yet), but yeah I had the same question.

This isn't an additional service? I guess I'm not really seeing what the downside to this approach is, when its just 4 lines to implement in each of our front-end apps and my understanding of how https calls work when we are just going over localhost and not the internet was that we don't really have to have the same reliability/speed concerns as we would over the internet but correct me if I'm wrong there.

I don't think its reasonable to move all of the types and log event id definitions into kiosk-browser as that would be a huge pain for development when we will likely be needed to add new log types pretty frequently alongside implementing other features. So we would still need to have the type definitions and the Logger class wrapper in vxsuite-complete-system, also so that we can reuse the same code for logging from the server-side, and we would then need to pass window.kiosk to the Logger to make that idea work which feels kind of convoluted to me, but I suppose it would be doable.

The other thing I'm unsure about in that approach if how confusing it might be when debugging things that the logs go to kiosk-browser and that you would need to be running the app through kiosk-browser (which I thought I'm the only person who regularly does?) in order to see the logs, which might be kind of annoying.

I can try to play around with this and make a PR for how that version would look but I guess I'm still just missing what the downside is with this approach.

The types part is a good point, though maybe it's just as simple as your setupProxy thing where it just forwards to console.log and doesn't care about the types. If not running in kiosk-browser it could just console.log. I'm not sure which approach I prefer. I'm curious to hear more from @benadida.

Yeah thats also what I was thinking we would do in the kiosk-browser approach.

Somewhat separately though from this decision kiosk-browser will probably need to make its own logs, so I'm not totally sure how to best handle that with the types, if I should just double-implement the kiosk-browser ones in kiosk-browser and vxsuite (so the UI reading this to display logs and the code to transform the logs to the CDF format knows the types), or if I should try to import the logging package into kiosk-browser somehow.

I've been thinking we should have CI publish from main with a version like 1.0.0-abcdefg using the git hash, that way we can reference monorepo things from the outside.

@carolinemodic good point on the types, yeah, I wouldn't want you to have to import them all into kiosk-browser just for this. I was thinking more along the lines of what @eventualbuddha describes, where kiosk-browser implements just a pipe of text to stdout.

Thinking about this some more... I think I care less than I originally thought about whether it's a kiosk-browser call vs. a /log call. I lean a little bit in the direction of kiosk-browser, because adding a /log call turns the server from a pure server of static content to one that does a little bit more than that... but that's just one data point, and if you prefer the /log approach @carolinemodic I'm comfortable with it.

Quick thought about kiosk-browser logging. We've build kiosk-browser as a generic tool, and I think it would be great if we keep it that way. I would suggest that the web content can register for certain events from kiosk-browser and, upon getting notified, can log it. A good example of this already happening is how we register for printers being connected/disconnected --> content gets notified and can /log it. I'm thinking we may be able to do that for any other events from kiosk-browser. What do you think @carolinemodic ? (Also, this could be an entirely different discussion that doesn't belong in this ticket.)

Bottom line:

I stll lean very slightly in the direction of a kiosk-browser .log API, but after hearing @carolinemodic out I'm fully comfortable with the /log API call instead. Please feel free to do whatever feels right to you @carolinemodic.

I think it might be best if we can keep kiosk-browser logging within the same framework: content notification and then the content logs. The good part of this is that we don't need to include the logging library in kiosk-browser, which I think is best left as a standalone system. But to be clear, I'm still happy to discuss this.

carolinemodic · 2021-10-26T22:39:05Z

libs/@types/kiosk-browser/kiosk-browser.d.ts

@@ -1,7 +1,7 @@
 declare namespace KioskBrowser {
  export interface BatteryInfo {
-    discharging: boolean
-    level: number // Number between 0–1
+    discharging: boolean;


why was the file not already semi-colon-ified?

carolinemodic · 2021-10-26T22:41:16Z

libs/logging/src/logger.ts

+      debug(logLine); // for internal debugging use log to the console
+      await this.kiosk.log(
+        JSON.stringify({
+          timeLogInitiated: Date.now().toString(),


@eventualbuddha With using kiosk-browser instead of a POST request is there still a risk that things get sent out of order? Do you think it's still important to have this extra time (the logs will also have the time the console.log call gets made)?

benadida · 2021-10-27T13:22:50Z

libs/logging/src/logEventIDs.ts

+  eventType: LogEventType.UserAction,
+  defaultMessage: 'Application has been configured for a new election.',
+  documentationMessage:
+    'Message that the user has configured current machine to a new election definition.',


maybe no need for Message that at the beginning of these documentation messages?

benadida · 2021-10-27T13:23:15Z

libs/logging/src/logEventIDs.ts

+  eventId: LogEventId.MachineBoot,
+  eventType: LogEventType.HardwareAction,
+  documentationMessage:
+    'Log that appears when the machine has successfully booted.',


same, maybe just "the machine has successfully booted."

benadida · 2021-10-27T13:23:55Z

libs/logging/src/logger.test.ts

+    eventId: LogEventId.MachineBoot,
+    eventType: LogEventType.HardwareAction,
+    user: 'system',
+    message: 'I come back stronger than a 90s trend',


carolinemodic requested review from eventualbuddha and benadida October 22, 2021 01:24

eventualbuddha reviewed Oct 22, 2021

View reviewed changes

carolinemodic force-pushed the caro/logging/poc branch 2 times, most recently from fc9b0b8 to f3bd548 Compare October 22, 2021 23:26

benadida reviewed Oct 25, 2021

View reviewed changes

carolinemodic force-pushed the caro/logging/poc branch from f3bd548 to 388a479 Compare October 26, 2021 22:35

carolinemodic commented Oct 26, 2021

View reviewed changes

carolinemodic force-pushed the caro/logging/poc branch from 388a479 to 37fc882 Compare October 26, 2021 22:39

carolinemodic commented Oct 26, 2021

View reviewed changes

carolinemodic force-pushed the caro/logging/poc branch 2 times, most recently from 5dbaa34 to 4cab445 Compare October 26, 2021 22:52

carolinemodic requested review from eventualbuddha and benadida October 26, 2021 23:00

benadida approved these changes Oct 27, 2021

View reviewed changes

carolinemodic force-pushed the caro/logging/poc branch from 4cab445 to d0d0a4a Compare October 28, 2021 17:17

feat(logging) add initial logging utils and use case

ba6b0d7

carolinemodic force-pushed the caro/logging/poc branch from d0d0a4a to ba6b0d7 Compare October 28, 2021 17:23

carolinemodic merged commit 3277387 into main Oct 28, 2021

carolinemodic deleted the caro/logging/poc branch October 28, 2021 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial application logging setup #1102

Initial application logging setup #1102

carolinemodic commented Oct 22, 2021 •

edited

Loading

eventualbuddha Oct 22, 2021

eventualbuddha Oct 22, 2021

eventualbuddha Oct 22, 2021

carolinemodic Oct 22, 2021

eventualbuddha Oct 22, 2021

carolinemodic Oct 22, 2021

eventualbuddha Oct 22, 2021

eventualbuddha Oct 22, 2021

eventualbuddha Oct 22, 2021

carolinemodic Oct 22, 2021

eventualbuddha Oct 22, 2021

carolinemodic Oct 22, 2021

benadida left a comment

benadida Oct 25, 2021

eventualbuddha Oct 25, 2021

carolinemodic Oct 25, 2021

eventualbuddha Oct 25, 2021

carolinemodic Oct 25, 2021

eventualbuddha Oct 25, 2021

benadida Oct 25, 2021

carolinemodic Oct 26, 2021

carolinemodic Oct 26, 2021

benadida Oct 27, 2021

benadida Oct 27, 2021

benadida Oct 27, 2021

	expect(calls[0][1]).toMatchObject(
	expect(calls[0]?.[1]).toMatchObject(

Initial application logging setup #1102

Initial application logging setup #1102

Conversation

carolinemodic commented Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benadida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolinemodic commented Oct 22, 2021 •

edited

Loading