[usm] service discovery, system-probe ignore configured services by name #30702

yuri-lipnesh · 2024-11-04T12:37:27Z

What does this PR do?

This code adds a hashmap to store processes that should be excluded from processing based on the service name.
This code adds excludes service name 'datadog-agent' from processing.

Motivation

USMON-1226
Some processes cannot be excluded based on the command name, which may be a generic name such as "agent".
In this case, a service can be excluded based on the service name. When a service name is identified, the process ID is stored in an internal table to exclude it from processing.

Describe how to test/QA your changes

This code passed service discovery unit test on local VM
PASS
ok github.com/DataDog/datadog-agent/pkg/collector/corechecks/servicediscovery/module 38.766s

Started couple of programs on local VM and dogfeeded to dddev environment.
Checked that services were reported on APM page "Identify and close your observability gaps"
Checked that services are not reported anymore after adding services to the list of bypassed services.

Possible Drawbacks / Trade-offs

Additional Notes

…og-agent

agent-platform-auto-pr · 2024-11-04T12:38:21Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=48395348 --os-family=ubuntu

Note: This applies to commit a777efa

guyarb · 2024-11-04T14:55:27Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go

@@ -0,0 +1,51 @@
+// Unless explicitly stated otherwise all files in this repository are licensed


why is it in a new file?

source file impl_linux.go already reached 600 lines, which I think it considered large file, I didn't want to increase this file further, this is why new file.

it is misleading to have methods of the same struct defined on multiple files
let's fold it back to the original file

guyarb · 2024-11-04T14:55:46Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go

+// ignoreServices is a list of service names that should not be reported as a service.
+var ignoreServices = map[string]struct{}{
+	"datadog-agent": {},
+}


why only datadog agent?

This is meantime only service defined in scope of the ticket to be bypassed.

The PR is pretty much irrelevant
Customers with the datadog agent don't set DD_SERVICE for us, and we don't do that by default

So I'm concerned the current implementation does not provide any actual value and impact

Do you want to delete this PR and close Jira ticket?

no, I want to improve the ticket description, and make the implementation better for the general case

I've updated Jira ticket and title of this PR. Now this PR allows to add excluded service name to system-probe config. The config has one default service "datadog-agent"

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go

pkg/collector/corechecks/servicediscovery/module/ignore_proc_test.go

pkg/collector/corechecks/servicediscovery/module/impl_linux.go

… addIgnoredPid(proc.Pid).

cit-pr-commenter · 2024-11-04T17:19:28Z

Regression Detector

guyarb · 2024-11-05T06:11:47Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go

+func (s *discovery) shouldIgnoreService(name string, proc *process.Process) bool {
+	s.mux.Lock()
+	_, found := ignoreServices[name]
+	if found {
+		s.ignorePids[proc.Pid] = true
+	}
+	s.mux.Unlock()
+
+	return found


a function called should.... should only report true/false, and not changing an internal state

I've changed this function

guyarb · 2024-11-05T06:12:45Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go

+	s.mux.Lock()
+	s.ignorePids[pid] = true
+	s.mux.Unlock()


Always prefer defered unlock, it guarantees future modifications won't miss the unlock

Suggested change

s.mux.Lock()

s.ignorePids[pid] = true

s.mux.Unlock()

s.mux.Lock()

defer s.mux.Unlock()

s.ignorePids[pid] = true

This is very short frequently used function, 'defer' instruction requires additional allocation, which is not necessary in this case. This function is not expected to grow significantly.

that's a very negligible optimization.
Please follow the effective go guidelines https://go.dev/doc/effective_go#defer
Those are meant to reduce human errors and mistakes

I've changed this function to use 'defer'

guyarb · 2024-11-05T06:13:12Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go

+
+// shouldIgnoreService saves pid of the process if the service should be excluded from handling
+// and returns true for such process.
+func (s *discovery) shouldIgnoreService(name string, proc *process.Process) bool {


no need to get Process here, if you care only about the PID

I've changed this code and separated ignored service and ignored pid

guyarb · 2024-11-05T06:16:52Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc_test.go

+			makeAlias(t, test.comm, serverBin)
+			bin := filepath.Join(serverDir, test.comm)
+			cmd := exec.CommandContext(ctx, bin)
+			cmd.Env = append(os.Environ(), test.envs)


why os.Environ?

guyarb · 2024-11-05T06:17:01Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc_test.go

+			err := cmd.Start()
+			require.NoError(t, err)


Suggested change

err := cmd.Start()

require.NoError(t, err)

require.NoError(t, cmd.Start())

guyarb · 2024-11-05T06:18:39Z

pkg/collector/corechecks/servicediscovery/module/ignore_proc_test.go

+				// wait until the service name becomes available
+				info, err := discovery.getServiceInfo(proc)
+				assert.NoError(collect, err)
+				assert.NotEmpty(collect, info.ddServiceName)


you're expecting for a certain value (based on the environment variable value), use it

this unit test intends to check that Pid has been bypassed, there are other unit test covering service name extraction. I do not think this unit test should check match of service name.

then why do you test ddServiceName?

I've added service name verification

pkg/collector/corechecks/servicediscovery/module/impl_linux.go

guyarb · 2024-11-05T06:22:43Z

pkg/collector/corechecks/servicediscovery/module/impl_linux.go

+	if s.shouldIgnoreService(name, proc) {
+		return nil
+	}


why don't you add the pid to the ignore list?

I've changed this code and separated ignored service and addition of ignored pid

guyarb · 2024-11-05T06:26:31Z

The ticket and PR should not focus merely on DD_SERVICE, but to filter entries by service name

Please fix the code, PR description and title accordingly

…rvices

guyarb · 2024-11-06T10:37:28Z

pkg/collector/corechecks/servicediscovery/module/config.go

+// loadIgnoredServices saves names that should not be reported as a service
+func (config *discoveryConfig) loadIgnoredServices(services []string) {
+	if len(services) == 0 {
+		log.Debug("loading ignored services found empty commands list")


copy-paste bug, commands list is inaccurate

… map[]struct{}

…trace-agent, process-agent, datadog-cluster-agent.

… tests.

yuri-lipnesh · 2024-11-07T17:42:30Z

/merge

dd-devflow · 2024-11-07T17:42:40Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-11-07 17:42:40 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2024-11-07 21:22:32 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 24m.

brycekahle · 2024-11-07T19:29:30Z

pkg/config/setup/system_probe.go

@@ -404,6 +404,7 @@ func InitSystemProbeConfig(cfg pkgconfigmodel.Config) {
 	cfg.BindEnvAndSetDefault(join(discoveryNS, "enabled"), false)
 	cfg.BindEnvAndSetDefault(join(discoveryNS, "cpu_usage_update_delay"), "60s")
 	cfg.BindEnvAndSetDefault(join(discoveryNS, "ignored_command_names"), []string{"chronyd", "cilium-agent", "containerd", "dhclient", "dockerd", "kubelet", "livenessprobe", "local-volume-pr", "sshd", "systemd"})
+	cfg.BindEnvAndSetDefault(join(discoveryNS, "ignored_services"), []string{"datadog-agent", "trace-agent", "process-agent", "datadog-cluster-agent"})


Add security-agent and other processes listed in cmd/*?

APM Distributed Tracing team provided us with some test environment with service discovery enabled, I took some DD services from that environment. Apparently security is not enabled there, that's why I don't see that service-agent in the discovery list. I would be careful about adding names that I don't know how they show up in discovery. This is why I prefer not to extend this list at this point.

@yuri-lipnesh - Bryce is mentioning another DD product that it is not enabled in your environment. Please open a follow up PR to address that, and run PR in dogfooding.

I've created USMON-1339

[usm] service discovery, system-probe ignore service DD_SERVICE=datad…

6fb5337

…og-agent

yuri-lipnesh requested a review from a team as a code owner November 4, 2024 12:37

github-actions bot added component/system-probe medium review PR review might take time labels Nov 4, 2024

yuri-lipnesh added changelog/no-changelog team/usm The USM team qa/done QA done before merge and regressions are covered by tests team/apm-onboarding labels Nov 4, 2024

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go Outdated Show resolved Hide resolved

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/collector/corechecks/servicediscovery/module/ignore_proc.go Outdated Show resolved Hide resolved

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/collector/corechecks/servicediscovery/module/ignore_proc_test.go Outdated Show resolved Hide resolved

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/collector/corechecks/servicediscovery/module/impl_linux.go Outdated Show resolved Hide resolved

[usm] service discovery, system-probe, if shouldIgnoreComm(proc) then…

4f621c6

… addIgnoredPid(proc.Pid).

github-actions bot added long review PR is complex, plan time to review it and removed medium review PR review might take time labels Nov 4, 2024

guyarb reviewed Nov 5, 2024

View reviewed changes

pkg/collector/corechecks/servicediscovery/module/impl_linux.go Show resolved Hide resolved

guyarb reviewed Nov 5, 2024

View reviewed changes

yuri-lipnesh changed the title ~~[usm] service discovery, system-probe ignore service DD_SERVICE=datadog-agent~~ [usm] service discovery, system-probe ignore discovered service datadog-agent Nov 5, 2024

[usm] service discovery, system-probe config add discovery.ignored_se…

98f36d0

…rvices

yuri-lipnesh requested a review from a team as a code owner November 5, 2024 18:17

yuri-lipnesh changed the title ~~[usm] service discovery, system-probe ignore discovered service datadog-agent~~ [usm] service discovery, system-probe ignore configured services by name Nov 5, 2024

guyarb reviewed Nov 6, 2024

View reviewed changes

[usm] service discovery, system-probe, change ignorePids map[]bool tp…

cf8b49b

… map[]struct{}

yuri-lipnesh requested a review from guyarb November 6, 2024 14:36

yuri-lipnesh added 2 commits November 6, 2024 16:04

[usm] service discovery, set default ignored services:datadog-agent, …

02044e8

…trace-agent, process-agent, datadog-cluster-agent.

[usm] service discovery, adjust default number of ignored services in…

a777efa

… tests.

guyarb approved these changes Nov 7, 2024

View reviewed changes

brycekahle reviewed Nov 7, 2024

View reviewed changes

yuri-lipnesh requested a review from brycekahle November 7, 2024 20:18

brycekahle approved these changes Nov 7, 2024

View reviewed changes

dd-mergequeue bot merged commit 2a4706d into main Nov 7, 2024
294 checks passed

dd-mergequeue bot deleted the yuri.lipnesh/USMON-1226_discovery_ignore_service branch November 7, 2024 21:45

github-actions bot added this to the 7.61.0 milestone Nov 7, 2024

		@@ -0,0 +1,51 @@
		// Unless explicitly stated otherwise all files in this repository are licensed

	err := cmd.Start()
	require.NoError(t, err)
	require.NoError(t, cmd.Start())

[usm] service discovery, system-probe ignore configured services by name #30702

[usm] service discovery, system-probe ignore configured services by name #30702

Conversation

yuri-lipnesh commented Nov 4, 2024 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

agent-platform-auto-pr bot commented Nov 4, 2024 • edited Loading

Test changes on VM

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

yuri-lipnesh Nov 4, 2024 • 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

cit-pr-commenter bot commented Nov 4, 2024 • edited Loading

Regression Detector

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

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

guyarb commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-lipnesh commented Nov 7, 2024

dd-devflow bot commented Nov 7, 2024 • edited Loading

Devflow running: /merge

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

yuri-lipnesh commented Nov 4, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 4, 2024 •

edited

Loading

yuri-lipnesh Nov 4, 2024 •

edited

Loading

cit-pr-commenter bot commented Nov 4, 2024 •

edited

Loading

dd-devflow bot commented Nov 7, 2024 •

edited

Loading

Devflow running: `/merge`