-
-
Notifications
You must be signed in to change notification settings - Fork 221
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(metrics): Add send-metric command #2063
Conversation
b40f9ce
to
1a39b52
Compare
1a39b52
to
61ada35
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 have only reviewed some of the files. Will continue reviewing the rest later
26ee405
to
d7b0ba8
Compare
Please also remember to rename this PR to "feat(metrics): ..." |
d7b0ba8
to
76c5cbc
Compare
b19bcb9
to
4815045
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.
Another partial review – I think I checked everything besides the integration tests. Mostly small changes/nitpicks, but I did find a bug related to string indexing which will need to be fixed
7bf8519
to
7cafaaf
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.
LGTM, I will leave the approval to Daniel since I don't have all the CLI context I would need to properly review this PR.
7cafaaf
to
d205688
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 added some comments, and requested some changes, primarily related to adding clearer user-facing documentation. This time, I looked through all the files (including integration tests) – some of the changes I requested are in the files changed for the integration tests.
Overall, this PR seems to be coming together really well, and the code looks overall very good!
006ead9
to
a716176
Compare
Add CLI command that can emit metrics. Fixes GH-2001
a716176
to
319733e
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.
just a couple of comments, rest looks good
a4b1867
to
74a1aa7
Compare
Add CLI command
send-metric
for emitting metrics to Sentry.Add new command-parser that uses clap's Derive API. Future commands should use this as the Derive API makes it:
-Easier to read, write, and modify arguments and commands.
-Easier to keep the argument declaration and reading of argument in sync.
-Easier to reuse shared arguments.
Fixes GH-2001