-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feat/164/json output #240
Feat/164/json output #240
Conversation
Signed-off-by: Moriarty <[email protected]>
TODO: * refactor and rename a a few things Signed-off-by: Moriarty <[email protected]>
Signed-off-by: Moriarty <[email protected]>
Signed-off-by: Moriarty <[email protected]>
NOTE: This is branched off the wallet endpoints already so that needs to be merged first Signed-off-by: Moriarty <[email protected]>
needs #162 to be merged first |
Signed-off-by: Moriarty <[email protected]>
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.
bit difficult to review, but it does not matter that much.
The JSON logger MUST not collide with the verbosity logs and it should just be "log the normal ouput in a json format with a nice key/value, thats it.
Input:
siera --json connection invite
Output:
{
"invitation_url": "https://...=",
"connection_id": "abcd...xyz"
}
crates/logger/src/lib.rs
Outdated
/// Log level json and nothing else | ||
Json, | ||
|
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.
Json should not be a log level. Outputting JSON should not collide with the existing verbosity logs.
crates/logger/src/macros.rs
Outdated
if ::siera_logger::LogLevel::Json == | ||
::siera_logger::STATE.read().unwrap().level | ||
{ | ||
println!($($arg)*); |
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 would be nice here if we format the data, so the user does not have to do this themselves.
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.
ah true. actually better to do this here. what I did now is pass a formatted data blob that is json to this macro by using the pretty_stringify_obj
function
hmmm ok. Wouldn't it make sense to only log the json if json is passed? Was thinking that if only a json value is printed one can easily use siera in a context where you get sth that is json. So if you pipe the output somewhere or use its output to be read by some other program it can load that into whatever datastructure represents json in that lang without having to do any sort of cleaning. Or what is your opinion on this? So what's happening now is that |
I understand the point of only showing JSON, and it should not give the default log output, but it should not remove the verbosity logs if specified. So basically the normal logger should check the state and if it is json we can prettify it (also give it custom keys) and log it like JSOn instead of normal. |
Ok I understand what you mean (I think). so then you'd get the normal logs according to verbosity wrapped in JSON and the result is under a key in there like 'result' or sth like that? |
No, just the normal logs like the invitation url is in json, the verbosity logs will always stay the same. Just the output that we always log (with log!()) will be json. |
Sorry if I'm being daft here. Then we still have the log output in there if doing json? then one would still need to filter somehow the verbosity stuff and separate that from the desired json?! I thought the point was to just get sth that is definitely JSON so it can just be used in unix pipe or some program? |
Well just don't have a verbosity flag :). The regular command just displays the "pipable" information. We also have a quiet flag, for stuff like this. |
Alright I think where this is going. What about the loading spinner then? How do you imagine handling that. As in I'd assume this is a nice feature when not using this to pipe, but when using this for pipe that - if I'm not mistaken - will be part of the output (and undesired) when wanting json. |
The loader is send to stderr as a convention within unix. |
ahhh okayyyyy - thanks for clarifying. |
Signed-off-by: Moriarty <[email protected]>
Alright, so changed to your suggestions ( if I got them coeectly). no printing JSON for by default for applicable. Quiet frankly, preferred the previous way (or a version of it) with the flag. |
Signed-off-by: Moriarty <[email protected]>
7d4956d
to
e058dd2
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.
I don't think you understood my point.
Normally we do not want JSON output and we just want it seperated by a new line, like with the connections(uuid \n invitationURL).
When we pass the --json
flag, we want the following:
{
"connection_id": "foo-bar",
"invitation_url": "https://..."
}
the default behaviour will not change, but it is just swapped with the JSOn logger.
crates/automations/src/automations/create_credential_definition.rs
Outdated
Show resolved
Hide resolved
Signed-off-by: Moriarty <[email protected]>
Signed-off-by: Moriarty <[email protected]>
2e48934
to
236c40c
Compare
Signed-off-by: Moriarty <[email protected]>
Is this ready for review? |
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.
Quite interested if the macro works. Did you test it? Do you have some output I can see?
cheers. Thanks for all the feedback. Will address asap |
@blu3beri addressed |
Signed-off-by: Moriarty <[email protected]>
bf58c6c
to
7b855f5
Compare
@@ -141,7 +141,7 @@ pub async fn parse_wallet_args( | |||
}; | |||
agent.create_local_did(options).await.map(|response| { | |||
loader.stop(); | |||
log_info!("Successfully created local DID: ",); | |||
log_info!("Successfully created local DID: {:?}", response.did); |
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 don't think we need debug printing for the did (I assume it is a string). If it is not a string we need to convert 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.
One more comment.
I'm with @morrieinmaas here. I think it's quite normal for CLIs / tools that when you enable json mode, ALL messages will be printed in json. This way you can parse the logs and make sense of it (structured logging). If you enable verbosity logs, these should also be logged as json imho. Some examples: for example tsed has the following structure:
Also leaving this: https://stackify.com/what-is-structured-logging-and-why-developers-need-it/ |
Yes, makes sense. I am not opposed to json verbosity logging, but my point was mainly that logging JSON must not mute the verbosity logs. I think we can address this in a separate PR as it would require reworking the whole logger. We would probably need to always pass a json / struct and create a normal message from that or log it as JSON. |
Or not pass json/struct and create json/struct on the fly with eg a timestamp if the/a key is missing but value is present. I'll have a look into how this can be 'formally' handled as in sticking to some convention |
@morrieinmaas what is the status of this PR? |
closes #164