Skip to content
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

Add flags to the sub command #1059

Merged
merged 6 commits into from
May 23, 2024
Merged

Add flags to the sub command #1059

merged 6 commits into from
May 23, 2024

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented May 21, 2024

Adds the following flags to nats sub:

  • --subjects-only only prints the subject for each message received
  • --timestamp adds a time stamp of when the message is received
  • --delta-time adds a time since the start of the command of when the message is received

@jnmoyne jnmoyne requested a review from ripienaar May 21, 2024 19:00
cli/sub_command.go Outdated Show resolved Hide resolved
cli/sub_command.go Outdated Show resolved Hide resolved
cli/sub_command.go Outdated Show resolved Hide resolved
} else {
fmt.Printf("[#%d] Received JetStream message: consumer: %s > %s / subject: %s / delivered: %d / consumer seq: %d / stream seq: %d\n", ctr, info.Stream(), info.Consumer(), msg.Subject, info.Delivered(), info.ConsumerSequence(), info.StreamSequence())
fmt.Printf("[#%d]%s Received JetStream message: consumer: %s > %s / subject: %s / delivered: %d / consumer seq: %d / stream seq: %d\n", ctr, timeStamp, info.Stream(), info.Consumer(), msg.Subject, info.Delivered(), info.ConsumerSequence(), info.StreamSequence())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be confusing here since the message itself have a timestamp from js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to me, delta-time can not be confused and even with timestamps you can measure the difference between the time the message was received and the timestamp in the message itself. Also if c.jetStream is false then there is no other timestamp printed so no confusion possible.
But I'm not strongly attached so I can remove it if c.jetStream is true if you want that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'd say lets print it only if its delta, that would be useful but haing timestamp here thats different from the message timestamp (which we arent showing) would definitely be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont follow, this one on line 566 still prints timestamp unconditionally

And we're duplicating the lines here just for the timestamp.

Lets just remove timestamp and delta completely from all jetstream related messages that seems the easiest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it from just the JS line that had a (JS) time stamp only (there's no JS data time stamp printed in the case c.jetStream is false).

I also thing in non-JS mode ( c.jetStream is false) it would look strange to not have the timestamp printed just because the message happens to be a JS message (!).

But if you prefer no timestamps at all for the JS stuff I just did that now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont mind it on the jetstream ones if its done as I requested, but if I cant have that I'd rather just not have it at all.

So for example, down in the PR we print a JS ACK subject where we have both the timestamp of the message and still the timestamp. This is confusing and what I keep asking to remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I had mis-understood what you were asking for then. How about how it is now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pleasse remove all timeStamp from an line anywhere that mentions jetstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so removed not just if c.jetStream

}

prettyPrintMsg(msg, c.headersOnly, c.translate)
if !c.sOnly {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather do if c.sOnly { return } and then dont create a big nested block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

prettyPrintMsg(msg, c.headersOnly, c.translate)
if !c.sOnly {
return
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of returning early is so that this else is not needed and so the code below remains unchanged and the extra indent isnt needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

- `--subjects-only` only prints the subject for each message received
- `--timestamp` adds a time stamp of when the message is received
- `--delta-time` adds a time since the start of the command of when the message is received

Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Copy link
Collaborator

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jnmoyne jnmoyne merged commit 8249277 into main May 23, 2024
4 checks passed
@jnmoyne jnmoyne deleted the jnm/21-may-24 branch May 23, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants