-
-
Notifications
You must be signed in to change notification settings - Fork 229
feat: implement unified JSON/text logging format #4392
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
base: master
Are you sure you want to change the base?
Conversation
|
Appreciate the feedback. I'll review these and make the requested changes. |
|
I see a few multi-line logs from the driver still. I'd like to see if I can get the raw structured version out - working on this now. |
|
I think I've structured all I can without driver changes. IIUC, The structured data gets permanently lost in the driver's logging pipeline when messageRecordToLines() converts the original MessageRecord objects (like {"source node id": 1, "callback id": 9}) into formatted string arrays (like ["source node id: 1", "callback id: 9"]) before the data ever reaches zwave-js-ui. I could lightly parse this into a single line so that centralized logging systems are grok parse it more easily. But, I think it might be better to remain unopinionated on that and perhaps do additional structuring in the driver itself. For example, here's a not-so-structured log from the driver: |
|
I double-checked. Formatting the messages happens outside of the transport format, in https://github.com/zwave-js/zwave-js/blob/266f47f49f804609d633313fe128a81c987732ff/packages/zwave-js/src/lib/log/Driver.ts#L113 We'd need to add an option |
|
I've added the driver option in zwave-js/zwave-js#8204 |
|
Great, thanks for adding that support. The log is now much more structured: Edit: Noticed |
| * Setup driver logging based on the configured format (text or JSON) | ||
| * Uses the same logFormat setting as the gateway for consistency | ||
| */ | ||
| private setupDriverLogging(zwaveOptions: PartialZWaveOptions) { |
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.
I think the code in this function is overcomplicated and introduces lot of repeated code
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.
I broke this up into more manageable functions and made it more DRY.
src/views/Debug.vue
Outdated
| // no need to make this reative | ||
| this.prevScrollTop = scrollTop | ||
| }, | ||
| formatLog(data) { |
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.
as said I would like to avoid this kind of parsing if possible on frontend side
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.
Right, I missed that sorry. I took Debug.vue back to original state and handled this via the zwave client. I left the original minimal formatting designed for text as it wasn't negatively impacting JSON.
63f355f to
b0c263f
Compare
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.
@AlCalzone Do you think this could be simplified ?
|
@robertsLando @AlCalzone - Are there any other requested changes / feedback on this? |
api/lib/ZwaveClient.ts
Outdated
| const parseFormat = this.createParseDriverJsonFormat() | ||
| const jsonFormat = winston.format.combine( | ||
| parseFormat(), | ||
| winston.format.timestamp(), |
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.
Have you tested if this is needed?
The ZWaveLogInfo objects from zwave-js should already contain a timestamp.
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.
I removed this, but as I was testing I noticed another issue. The UI logs are in UTC with timezone indication. However, the driver logs are in local time with no timezone indication. I could convert this, but I think this is probably a concern of the driver to support UTC? The challenge isn't so much the usage of localtime, but rather the lack of TZ info.
Do you agree that the driver should at least use a format with TZ, and possibly also be UTC?
| :items="logFormats" | ||
| v-model="newGateway.logFormat" | ||
| label="Log Format" | ||
| hint="Choose between human-readable text or structured JSON logging" |
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.
I'm a bit afraid that making users choose hinders our troubleshooting capabilities. Everything from the automated bot comments when users post the wrong log, over the automatic log analyzer, to my feeble brain expects the text-based logging format.
Can we instead just add the JSON based logs as an additional transport that can be toggled?
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.
I'm trying to think about how best to do this. My use case for JSON logs is to ship them to centralized logging tools - ideally this reads the STDOUT of the container/process. Alternatively, it would read known files, but that's less ideal as the files then need to be specified whereas STDOUT is assumed.
I see no problem in always making the web console text format. I actually almost did that, but decided consistency was better. But, I think it would make sense to do so.
Where I'm stuck is - what is your expectation of the format of STDOUT and file logs in the case where the additional JSON transport is toggled on?
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.
I think configuring the STDOUT should be fine. Our most common troubleshooting use case is having users write debug logs to file and uploading those files.
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.
It just occurred to me that this presents a conflict I believe:
Driver's raw option is global - it affects ALL transports (console, file, web UI)
- When raw: true -> JSON for everything (loses human-readable formatting)
- When raw: false -> Text for everything (can't get JSON console)
We could move human parsing logic to the UI, but that's duplicative to what the driver already does in text mode so it feels a bit hacky. Is there another way to solve this that I'm missing?
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.
I guess I'll have to tie the message formatting into the transport format, so it can be toggled on a per-transport basis.
I've raised zwave-js/zwave-js#8375 for this. The solution probably involves getting rid of the raw option again though, so this PR should be considered blocked by that.
- Add support for JSON logging format alongside existing text format - JSON mode outputs structured JSON logs to console, files, and debug view - Text mode continues to output human-readable text with ANSI colors - Debug view displays raw format as received (JSON strings or formatted text) - Logging format configurable via gateway settings (logFormat: 'json' | 'text') - Both formats use consistent JSONTransport architecture - Maintains backward compatibility with existing text logging behavior This enables structured logging for better log parsing, monitoring, and integration with log aggregation systems while preserving the existing text format for human readability.
|
@AlCalzone I think I've addressed everything with the exception of one open question:
|
This provides a unified logging system where users can switch between human-readable text logs and structured JSON logs across all output destinations: console, files, and WebSocket/UI debug view.
Why do I write a /dev/null file in JSON mode?
The zwave-js driver's internal logic creates a console transport when
(isTTY && !logToFile)is true, but we neededlogToFile: trueto prevent console output in JSON mode. However, settinglogToFile: truealso creates an unwanted text file transport that writes to the same filename as our JSON logs, causing duplicate/conflicting file output.Solution: I redirected the driver's internal file transport to
/dev/nullusingfilename: /dev/nullin the driver'slogConfig, which satisfies the driver's internal logic (preventing console transport creation) while discarding the unwanted text file output, allowing our custom JSON file transport to handle the actual file logging cleanly.