-
Notifications
You must be signed in to change notification settings - Fork 5
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
CLI option --format=json
should change the STDERR logging to output JSON structured logging
#421
Conversation
👇 Click on the image for a new way to code review
Legend |
13d10e1
to
f3a028c
Compare
Should this PR only update |
Only js-logger. The |
4205327
to
96777ad
Compare
This PR should now be ready for review/merge. All of the |
I think you need to set the formatter in The formatting configuration should be propagated to the agent when it starts. Do you have some tests for this change? Also can you show a screenshot of it being executed and showing up JSON logs? |
If this is merged first @tegefaulkes will there be any conflicts? |
--format=json
CLI option affects logger output--format=json
should change the STDERR logging to output JSON structured logging
Added this in now
Not yet, but I'll write some now
Sure, here's an agent starting and stopping with JSON logs
|
3651d65
to
037ef6e
Compare
I've rebased on staging and added a test for logger formatting. This should be good to merge now. |
- Reverted Windows to NodeJS v16.14.2 - Added `--arg ci true` to `nix-shell` - Introduced continuous benchmarking
037ef6e
to
dca4cca
Compare
import * as execUtils from '../utils/exec'; | ||
import { runTestIfPlatforms } from '../utils'; | ||
|
||
describe('polykey', () => { | ||
runTestIfPlatforms('lunix', 'docker')('default help display', async () => { | ||
runTestIfPlatforms('linux', 'docker')('default help display', async () => { |
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.
@tegefaulkes why does this require a conditional check?
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 should've been removed. I must've missed it.
const result = await execUtils.pkStdio([]); | ||
expect(result.exitCode).toBe(0); | ||
expect(result.stdout).toBe(''); | ||
expect(result.stderr.length > 0).toBe(true); | ||
}); | ||
runTestIfPlatforms('docker')('format option affects STDERR', async () => { |
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.
Same here, does this mean this test only runs under docker platform?
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 got a little confused by the wording but that test runs both locally and in the testing phase of the CI/CD so I think it means if the platform is docker
then run it (but if the platform isn't specified also run it).
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.
Ok I think the wording needs to be changed in a commit in staging. It should be a strict AND
logic and OR logic between conditions.
This is why it shouldn't have changed to just a string list. A Boolean conditional is more obvious and flexible.
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 runs by default if the platform isn't specified. This is because we need to run all tests during normal testing and I'd rather not have to set PK_TEST_PLATFORM
for that. I think we can make it non-implicit by using runTestIfPlatforms(undefined, 'docker')
?
Ok merging, but I think there needs to be a change for the run condition utility functions later. |
The solution is reify the booleans like I do in the js-logger with trace feature detection. Along with the original functional abstraction as compared to taking a Boolean. Function abstraction can take test metadata if necessary. But I might just stick with the original Boolean parameter too and just use`&&` with reified booleans.
On 28 July 2022 11:07:54 am AEST, Brian Botha ***@***.***> wrote:
***@***.*** commented on this pull request.
> const result = await execUtils.pkStdio([]);
expect(result.exitCode).toBe(0);
expect(result.stdout).toBe('');
expect(result.stderr.length > 0).toBe(true);
});
+ runTestIfPlatforms('docker')('format option affects STDERR', async () => {
It runs be default if the platform isn't specified. This is because we need to run all tests during normal testing and I'd rather not have to set `PK_TEST_PLATFORM` for that. I think we can make it non-implicit by using `runTestIfPlatforms(undefined, 'docker')`?
--
Reply to this email directly or view it on GitHub:
#421 (comment)
You are receiving this because you modified the open/close state.
Message ID: ***@***.***>
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Description
When
--format=json
is specified on the CLI this should propagate to logger output (i.e.INFO
/WARN
/ERROR
/DEBUG
messages). With the newjs-logger
upgrade to 3.0.0 (MatrixAI/js-logger#14) this is now possible.Issues Fixed
--verbosity
flags Polykey-CLI#17Tasks
js-logger
to 3.0.0 (as well as for dependencies)formatting.jsonFormatter
option for loggers when--format=json
is set on the CLIFinal checklist