Adding the possibility to translate the msg.Data before output.

[oderwat]

We have decided to use the nats-cli tool to assist in debugging and monitoring NATS stream messages stored in cbor format. However, we would prefer the messages displayed in an easier-to-read formatting. The same goes for JSON, which we would like to have a better readable representation too.

In response to my inquiry #636 about adding an encoder/decoder, @ripienaar suggested that this could be achieved by passing the output through a CLI utility that performs the translation. This would allow users to add their own translators or formatters as needed.

I have created a proof of concept implementation for this approach, and I hope that it will be incorporated into the tool in the future.

I added a template for {{subject}} which can be used to let the translator know about the subject, so one could write it in a way that it uses internal knowledge to assist in decoding the data without the need to add information to the messages. There could also be a template for header fields or maybe also other information (the stream name, for example). I didn't want to add much more until the general idea gets the green light.

[ripienaar]

This is pretty cool, good use case examples also.

I left one comment there let me know what you think otherwise I will take a closer look next week

[oderwat]

I added a 'stream' template and pondered about adding the headers, as one could use them especially to describe the data.

I am also unsure of the argument name "translate", in our code base this would probably something like "cvt" or "flt".

The following is not part of this PR

I noticed that the code has many missing error checks ("will never happen" / "if that breaks, I am fleeing the country already"). We made a simple "must" package (using generics) to create must.Ok(), must.OkOne(), must.OkSkipOne() (and so on) which simply check and panic or return/ignore one return value.

We also have an alternative package that checks for the errors and on error lists just the file and line of the caller in the source and os.Exit(1). This is what we use for CLI tooling that is used by people that freak out when they get a panic with stack-dump. :)

This way it is effortless and readable to cover all error checks without having all the single if statements for the error checks. I would suggest adding something like this and could volunteer.

[ripienaar]

Hmm mind pointing at a few missing error checks?

We should probsbly add templates for all the jetstream metadata, not sure it becomes quite hard to use - my first thought was just to dumb the message as json but your examples are quite compelling and json woukd CI okicsge matters in your examples.

[oderwat]

Hmm mind pointing at a few missing error checks?

There are quite a lot

[ripienaar]

That’s a test :) fairly acceptable imo as following assertions would not pass had there been an error.

There are in general vast amounts of unchecked errors you are right - like fmt.Printf returns unchecked errors. So I would be curious if one’s if actual consequence outside of tests.

[oderwat]

As usual, adding something like that opens up plenty of ideas. I would rather go with a minimalistic approach first and see what emerges from that. It should be easy to add more later on. For adding something like JSON information, one may create a temp file with that JSON, add the filename as template and remove the JSON after the translator has run.

[oderwat]

That’s a test :) fairly acceptable imo as following assertions would not pass had there been an error.

I would add them, especially in tests. That makes it easier to see where it breaks if it breaks.

This is what I get when I quickly filter out '*_test.go':

This only comes to my mind because we have a zero warning policy for all code (using GoLand) and I also plan a global golangci-lint config to be mandatory for all pushed code into the CI/CD chains. But I know that I am quite strict with that in our company. We should have more tests though :)

P.S.: This is basically the reason why I allow a 'must' package which panics. I also had quite some trouble using some NATS example code, made some modifications and did not notice that it was breaking and there were no error checks in the examples. I am uncertain if that was from the documentation or Stack Overflow, though.

[ripienaar]

In this case I would fetch that command. It’s old old code that was just copy and pasted in here.

I wouldn’t want to change the code style at all. Rather just add checks where they are of consequence.

update: bench, RTT, latency, early sub, pub, pub, reply and maybe a few other bits were pulled from loose utilities. In those we can just add idiomatic error handling.

[oderwat]

I really didn't want to steer up the error handling stuff. As mentioned earlier. This PR was made to extend the ways of using the wonderful nats-cli tool with data that's encoded in some binary format. I actually wrote some "universal translator" implementation today that also uses AVRO model info stored in the stream to decode messages. It helps already by allowing to use "hex", "raw escaped strings", "CBOR", "AVRO" and "JSON" formats. Using "GOB" turned out to neither be smaller than JSON nor is debug printable without recompiling the tools and adding all the structures (or writing a debugging decoder from scratch). I didn't use AVRO before, but found that this may be the best way for us to store large amounts of data consisting of many messages (which makes gob rather bloated). It seems that Kafka has a point in using it. The possibility to store the structure as text and don't need to recompile when updating the format, makes it quite interesting when storing many messages with the same (binary) format.

[oderwat]

About that "must" package:

I like how the code looks when using it while prototyping or even for production:

dec := cbor.NewDecoder(os.Stdin)
var cborNative any
must.Ok(dec.Decode(&cborNative))
mappedKeys := must.OkOne(convertMapKeysToStrings(cborNative))
jsonData := must.OkOne(json.MarshalIndent(mappedKeys, "", "  "))
fmt.Printf("%s\n", jsonData)

We have now two packages. One that just uses panic() for the errors and another one that uses log/slog and os.Exit().
Because of this conversation, I decided to make them both exchangeable. I find that much better than having the if(err) { panic(err) }. We do forbid variable shadowing too, and as a result, you need to name many of your errors when using just "err" all the time.

[ripienaar]

I'll review the data translation towards end of week, thank you.

I would not be interested in anything like the must package here, happy to have PRs adding idiomatic go error handling where its lacking now.

[ripienaar]

elsewhere we use text/template for this and people would already be used to being able to put spaces in and so forth. So lets also use text/template here and then make it Subject and Stream also for consistency with like whats in use in nats req and so forth wrt capitalization

[ripienaar]

Using text/template also means we can easily extend later

[oderwat]

I need to read up in text/template. I never used that myself.

[oderwat]

I changed it to use the text/template package. Please check. I wonder about how there is sometimes {{.Count}} (README.me:145) but mostly {{Count}} in the documentation. Am I supposed to use the functions instead of the fields or doing both, what I think that is being done in pubReplyBodyTemplate()?

[ripienaar]

I think the readme is probably wrong. Can pass in a map[string]any wirh those fields is enough and then highlighting them in help output.

[oderwat]

Do you mean additionally to the {{.Subject}} or instead? I ask because pubReplyBodyTemplate() allows both variants {{Count}} and {{.Count}}.

[ripienaar]

you can pick one, which ever you like - the pub/reply one is complex because of a early backward incompatible change and so i made it kind of work. since yours is new you can just go with one as long as its in --help

[oderwat]

Well. I picked the {{.Subject}} type and updated the README with the last commit already. The reason was that using {{.Subject}} instead of {{Subject}} may people aware of that it is using the text/template and let them use other functionality of it. But I got unsure because most of the documentation does not use this variant and the additional . may make people wonder. So, the {{Subject}} variant it will be then. The documentation was updated.

[ripienaar]

looks good thanks, any chance we can get that duplicated code pulled into a seperate function?

[oderwat]

I am unsure what duplicate code you refer to.

[oderwat]

OK. I consolidated the code and think that the generalized output function should result in the same user experience while unifying the edge cases (like nil message body). I did not rename the source file which contains that code to make it easier to see my changes. I guess it should be renamed to "output_msg_body.go" before merging.

What do you think about it overall now?

[ripienaar]

Good stuff thank you, do you mind signing this with gpg or your ssh key? Please squash to one commit and sign that commit.

[oderwat]

Good stuff thank you, do you mind signing this with gpg or your ssh key? Please squash to one commit and sign that commit.

I actually never used signing before. I hope I did that right.

[ripienaar]

Yup signature looks good

[ripienaar]

Thank you