Add --trace flag to CLI commands #406

Merged
fyrchik merged 3 commits from fyrchik/frostfs-node:tracing-cli into master 2023-06-02 11:44:06 +00:00
Owner

The task proved to be more difficult than I initially thought, I am not yet satisfied with the solution, thus WIP.

Ideally, we would like automatically have --trace flag for all commands which use --rpc-endpoint (or --endpoint for a control service) flag.
This is not completely feasible:

  1. Because we exit with an OS syscall, trace id won't be printed on command errors, but this could be sth we want to do (e.g. commands failed with the timeout).
  2. Some commands use JSON output, we need to take this into account.

So the current solution is to have 2 functions: start and print trace. Their inner logic depends on whether --trace flag was provided.
If a command has tracing enabled, it MUST register these handlers in Pre/Post run functions.

May be it is better to defer in every function which prints trace: this way we can have more fine-graineg control.
Another option is to cover on a case-by-case basis, but I see this flag useful and "polished" only if done simultaneously for all commands, you don't always new what you want to trace.

The task proved to be more difficult than I initially thought, I am not yet satisfied with the solution, thus WIP. Ideally, we would like automatically have `--trace` flag for all commands which use `--rpc-endpoint` (or `--endpoint` for a control service) flag. This is not completely feasible: 1. Because we exit with an OS syscall, trace id won't be printed on command errors, but this _could_ be sth we want to do (e.g. commands failed with the timeout). 2. Some commands use JSON output, we need to take this into account. So the current solution is to have 2 functions: start and print trace. Their inner logic depends on whether --trace flag was provided. If a command has tracing enabled, it MUST register these handlers in Pre/Post run functions. May be it is better to defer in every function which prints trace: this way we can have more fine-graineg control. Another option is to cover on a case-by-case basis, but I see this flag useful and "polished" only if done simultaneously for all commands, you don't always new what you want to trace.
fyrchik requested review from storage-core-committers 2023-05-29 09:44:30 +00:00
fyrchik requested review from storage-core-developers 2023-05-29 09:44:30 +00:00
dstepanov-yadro reviewed 2023-05-30 13:20:48 +00:00
@ -0,0 +17,4 @@
type spanKey struct{}
// StartClientCommandSpan stops tracing span for the command and prints trace ID on the standard output.
func StopClientCommandSpan(cmd *cobra.Command, opts ...trace.SpanEndOption) {

opts ...trace.SpanEndOption is unused arg. I think it can be deleted.

`opts ...trace.SpanEndOption` is unused arg. I think it can be deleted.
Author
Owner

Fixed.

Fixed.

func ExitOnErr(cmd *cobra.Command, errFmt string, err error) is called in the most of commands to exit with error. So you can place here trace id printing.

`func ExitOnErr(cmd *cobra.Command, errFmt string, err error)` is called in the most of commands to exit with error. So you can place here trace id printing.
fyrchik force-pushed tracing-cli from d8de1fc8d9 to 48394ab285 2023-06-01 14:27:48 +00:00 Compare
fyrchik force-pushed tracing-cli from 48394ab285 to d72bace127 2023-06-01 14:28:58 +00:00 Compare
fyrchik changed title from WIP: Add --trace flag to CLI commands to Add --trace flag to CLI commands 2023-06-01 14:29:12 +00:00
dstepanov-yadro reviewed 2023-06-02 07:49:49 +00:00
go.mod Outdated
@ -121,2 +121,4 @@
v1.22.0 // Published accidentally.
)
replace git.frostfs.info/TrueCloudLab/frostfs-api-go/v2 => ../api-go

drop

drop
Author
Owner

Fixed

Fixed
acid-ant approved these changes 2023-06-02 08:53:09 +00:00
@ -0,0 +16,4 @@
type spanKey struct{}
// StartClientCommandSpan stops tracing span for the command and prints trace ID on the standard output.
Member

StartClientCommandSpan -> StopClientCommandSpan

StartClientCommandSpan -> StopClientCommandSpan
Author
Owner

Fixed

Fixed
fyrchik force-pushed tracing-cli from d72bace127 to 6c02959fd2 2023-06-02 09:15:11 +00:00 Compare
dstepanov-yadro approved these changes 2023-06-02 11:15:31 +00:00
fyrchik merged commit f1f56ef871 into master 2023-06-02 11:44:06 +00:00
fyrchik referenced this pull request from a commit 2023-06-02 11:44:06 +00:00
fyrchik deleted branch tracing-cli 2023-06-02 11:44:07 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#406
No description provided.