Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 a new "vault monitor" command #8477
Add a new "vault monitor" command #8477
Changes from 53 commits
2549fde
095c161
abe7b0b
700a7e1
eefda16
c4f0c6b
aea83a3
e685d3e
ec69fcd
10c56e7
2527448
838abcd
78f3c0c
61ae825
60fb41c
f264bb3
f8c7805
4375fbb
4c55ab5
1e92efb
5cabeb8
e03ab93
cc3fadc
92f2148
eff3405
8da04c1
c3a7aa2
2a073af
faaec8b
02f9624
fa56c60
4067ff3
78092aa
1e7b56a
85f6f52
dc5a636
ac82aee
da2d044
a545a8d
0f80a2a
450d3cc
5ec427c
596eaaf
d2a74b3
2096a05
97bd34f
5cdf56d
061c211
e11a423
5c97324
1182914
c0483ad
7b53da8
c6081b6
35639e8
8f8d3e8
37915a4
2b18c98
46d60d2
b824167
c9e2138
98bae2e
dd60ee1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 good to put this in a
select
statement with a default so that if the buffer is full for some reason we can still cancel the command via canceling the context (and potentially print some kind of error indicating that messages were dropped). If you want to get fancy you could buffer the messages internally and drain when the channel unblocks; another fancy thing would be if you have a default statement and keep track of how many times you hit it, then if the function quits out (or the channel unblocks) you can print a note indicating that X messages were dropped.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.
All of this sounds really good, but I think much of the functionality you describe already exists on the server side, in helper/monitor/monitor.go. Is there any way to share that functionality between these two locations? Is it valuable to be checking for dropped messages on both the client and server side? I don't mean to second-guess your judgement - mostly I'm just curious if there's a way to avoid duplication of effort/code (or if that's just the nature of this particular beast).
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.
This pattern is on the server side too but it's running in two totally different contexts. You don't want to block the server because of this, but you also don't want to block the client because of this. For instance, if someone hits Ctrl-C while logCh is full, you don't want the command to just hang until text comes in, you want it to actually exit.
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.
Gotcha, makes sense. I'll see if there's a way I can abstract out the details of all of this so it can be used in both places.
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.
Do these completions work case insensitively?
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 not sure. I think I forgot to change those to lowercase when I changed the rest. I'll update them.
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.
We may want to update this so we do not care about case. Elsewhere were allow
trace
to be specified.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.
Good call
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.
Yeah generally we use lowercase not uppercase for log levels in commands, so if we're going to print valid levels out we should print out lowercase levels, not uppercase.