cli: enable/disable specific debug targets #11081

davemay99 · 2021-08-25T03:34:22Z

No description provided.

DerekStrickland

All in all, great work. You write much better go than I do. I left a lot of comments, but that is not indicative of any misgivings as much as feedback. Great job!

DerekStrickland · 2021-08-25T08:17:41Z

command/operator_debug.go

+}
+
+// IsValid checks whether the key exists in the map
+func (m targetMap) IsValid(value string) bool {


Does this need to be exported? And should targetMap be *targetMap? Also, might this be better named Exists?

Good catch -- definitely no need to export.

Since maps are already implicitly byref in nature, I think a value receiver makes the most sense here.

As for the function naming, you're right, this is a bit confusing. I initially had a separation between "supported targets" (the list of all explicit targets that can be specified) versus "valid targets" (which also includes the meta sets "all" and "none"). But I stripped it out at the last minute before posting the draft PR.

Will reintroduce the "supported target" concept, which should clear up the distinction and improve readability.

DerekStrickland · 2021-08-25T08:23:00Z

command/operator_debug.go

+			continue
+		}
+
+		enabled = true


This is funny. Here we have 2 general programming principles at odds with each other namely default variables to negative case vs. direct boolean function calls. Nothing to change, but just a funny observation.

DerekStrickland · 2021-08-25T08:27:58Z

command/operator_debug.go

+			t = strings.TrimPrefix(t, "-")
+			enabled = false
+		}
+		if !ret.IsValid(t) {


If IsValid is only used to check if something does not exist, then you might consider renaming it to DoesNotExist, and getting rid of this !MethodCall. The not symbol before boolean functions hurts readability, and I've always been taught to avoid it if at all possible. However, sometimes it's not possible.

Good point on readability -- will address in upcoming commit.

command/operator_debug.go

DerekStrickland · 2021-08-25T08:32:22Z

command/operator_debug.go

+}
+
+// parseTargets parses a comma delimited target string into a targetMap
+func (c *OperatorDebugCommand) parseIntervalTargets(intervalTargets string) targetMap {


Same pointer observation on return val. Also, this appears to be virtually identical to the method above. I wonder if you couldn't extract a template method, and reduce duplication of logic.

Good catch - was going to refactor these in a later PR but I'll push a commit shortly to take care of it now.

DerekStrickland · 2021-08-25T08:54:49Z

command/operator_debug.go

+
+	// Fetch data from vault
+	if c.targetEnabled("vault") {
+		vaultAddr := getVaultAddrFromSelf(self)


Does this need similar logic as the consul stuff in the preceding lines, or is the interface different?

command/operator_debug.go

command/operator_debug_test.go

website/content/docs/commands/operator/debug.mdx

notnoop

@DerekStrickland made many good points and I'm adding more :P. I'm giving some conflicting advice here - In places I suggested getting rid of functions, but made nit-picky comments about their content mostly for guiding style.

A question about whether we should track targets vs interval-targets separately and make users track the distinction.

notnoop · 2021-08-30T20:26:19Z

command/operator_debug.go

+	serverIDs       []string
+	targets         targetMap
+	intervalTargets targetMap
+	targetLock      sync.Mutex


I don't believe you need this lock. targets and intervalTargets is set early on in Run() and isn't modified afterwards, so it doesn't need a mutex.

You are correct -- I was exploring options for enabling or disabling targets based on what is discovered during the capture (OSS vs Enterprise, what features are detected in the configuration, etc). I'll remove the lock and revisit if that functionality is added in the future.

notnoop · 2021-08-30T20:28:24Z

command/operator_debug.go

+// String returns a CSV of valid enabled targets
+func (t targetMap) String() string {


This struct and its methods live among OperatorDebugCommand methods, breaking the flow of methods and making it a bit harder to follow. I'd suggest moving targets at the end of the file or so.

Agreed -- good point

notnoop · 2021-08-30T20:30:17Z

command/operator_debug.go

+	var targetString string
+
+	for target, enabled := range t {
+		if enabled && target != "all" {


Why not emit all? If all isn't supposed to be a key here, then this skip seems like papering over a bug and makes it harder to understand what's going on.

notnoop · 2021-08-30T20:31:23Z

command/operator_debug.go

+	targetString = strings.Join(targets, ",")
+
+	return targetString


Generally, I'd suggest avoiding declaring variables so far away from where they are used. Also, you probably can get away with avoiding declaring the variable completely:

Suggested change

targetString = strings.Join(targets, ",")

return targetString

return strings.Join(targets, ",")

notnoop · 2021-08-30T20:34:47Z

command/operator_debug.go

+	}
+
+	// Enable or disable targets in map based on user input CSV
+	var enabled bool


enabled seems to be only used inside the loop, you don't need to declare it outside.

notnoop · 2021-08-30T20:54:44Z

command/operator_debug.go

+// targetMap generic map for targets
+type targetMap map[string]bool


The comment doesn't add more info than the type name IMO. Also, I wonder if it makes sense to drop Map here; is calling it targets ok here?

Suggested change

// targetMap generic map for targets

type targetMap map[string]bool

// targets is a wrapped set to capture enabled debugging targets

type targets map[string]bool

notnoop · 2021-08-30T20:55:34Z

command/operator_debug.go

+			targets = append(targets, target)
+		}
+	}
+	sort.Strings(targets)


👍 on sorting for repeatability and consistency.

notnoop · 2021-08-30T21:00:56Z

command/operator_debug.go

+	// Build starting target map
+	switch {
+	case targets == "":
+		return ret


Should we return targetDefaultsNone() here? If we return an empty uninitialized targetMap, I fear that follow up calls with IsValid will generate noisy invalid keys messages because the map is empty.

notnoop · 2021-08-30T21:02:17Z

command/operator_debug.go

+	case targets == "":
+		return ret
+	case strings.Contains(targets, "none"):
+		ret = targetDefaultsNone()


What should happen when a user passed none, allocations? Should we generate an error? Should we just return targetDefaultsNone() here as well without further processing?

As a draft PR I hadn't fully explained the concept yet, but the idea here is that the target flags are additive / subtractive. If you pass no target flag at all, then the default behavior is the same as it was before and "all" targets are enabled. To clarify I will add a description to the PR and expand the documentation in a future commit.

To your question, none,allocations should result in a map containing only allocations. To verify and help self-document I will add a test to ensure that this is always the case.

As another example, I have an upcoming PR which adds an eventstream target that will be disabled by default. To add this target to a capture you would use -target=eventstream. This results in a map containing all target defaults plus eventstream. Similar to your question, if you wanted only eventstream then you would use -target=none,eventstream.

notnoop · 2021-08-30T21:04:35Z

command/operator_debug.go

+  -targets=<metrics,-metrics,...,all,none>
+    Comma separated list of targets to capture.  Defaults to "all".
+    "-<target>" disables capture for that target.
+
+  -interval-targets=<metrics,-metrics,...,all,none>
+    Comma separated list of targets to capture during each interval.  Defaults
+    to "all".  "-<target>" disables capture for that target.


Do we document which targets are intervals and which ones aren't? Would it make sense to just have a single -targets argument without having users worry about the distinction?

hashicorp-cla · 2022-03-12T17:04:53Z

All committers have signed the CLA.

tgross · 2022-09-30T18:40:05Z

@davemay99 this PR is over a year old at this point. Do we want to close this or would you like someone on the team to try to carry it for 1.4.x?

davemay99 added 10 commits August 24, 2021 23:12

separate consul and vault address parsing

c3b4f7e

Rename argNodes to splitArgumentList

c3169ef

cli: Add target selection to nomad operator debug

3983d9b

Add target docs

8d56b70

Enable all targets by default

a00e0dc

Ensure targets are disabled

05b6e62

List targets in capture summary

46f6de0

Allow test cases with multiple errors

9929f61

Add optional debug flag for ease of testing

4fbcfef

Remove deadcode

953639f

vercel bot deployed to Preview – nomad-storybook-and-ui August 25, 2021 03:39 View deployment

vercel bot temporarily deployed to Preview – nomad August 25, 2021 03:39 Inactive

DerekStrickland reviewed Aug 25, 2021

View reviewed changes

notnoop reviewed Aug 30, 2021

View reviewed changes

Update target flag descriptions for readability

0b57358

vercel bot deployed to Preview – nomad August 30, 2021 21:12 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui August 30, 2021 21:12 View deployment

tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: enable/disable specific debug targets #11081

cli: enable/disable specific debug targets #11081

davemay99 commented Aug 25, 2021

DerekStrickland left a comment

DerekStrickland Aug 25, 2021

davemay99 Aug 30, 2021 •

edited

Loading

DerekStrickland Aug 25, 2021

DerekStrickland Aug 25, 2021

davemay99 Aug 30, 2021

DerekStrickland Aug 25, 2021

davemay99 Aug 30, 2021

DerekStrickland Aug 25, 2021

notnoop left a comment

notnoop Aug 30, 2021

davemay99 Oct 6, 2021

notnoop Aug 30, 2021

davemay99 Aug 31, 2021

notnoop Aug 30, 2021

notnoop Aug 30, 2021

notnoop Aug 30, 2021

notnoop Aug 30, 2021

notnoop Aug 30, 2021

notnoop Aug 30, 2021

notnoop Aug 30, 2021

davemay99 Aug 31, 2021

notnoop Aug 30, 2021

hashicorp-cla commented Mar 12, 2022 •

edited

Loading

tgross commented Sep 30, 2022

		// String returns a CSV of valid enabled targets
		func (t targetMap) String() string {

		targetString = strings.Join(targets, ",")

		return targetString

		// targetMap generic map for targets
		type targetMap map[string]bool

cli: enable/disable specific debug targets #11081

Are you sure you want to change the base?

cli: enable/disable specific debug targets #11081

Conversation

davemay99 commented Aug 25, 2021

DerekStrickland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemay99 Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashicorp-cla commented Mar 12, 2022 • edited Loading

tgross commented Sep 30, 2022

davemay99 Aug 30, 2021 •

edited

Loading

hashicorp-cla commented Mar 12, 2022 •

edited

Loading