Add Host Observer #432

asuresh4 · 2020-07-13T22:43:17Z

Depends on #427

Description: Add host observer to look for listening network endpoints on host

Testing: Unit tests added. Manually tested with receivercreator receiver.

Documentation: README.md.

codecov · 2020-07-14T15:33:16Z

Codecov Report

Merging #432 into master will decrease coverage by 0.09%.
The diff coverage is 90.66%.

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
- Coverage   85.96%   85.87%   -0.10%     
==========================================
  Files         187      189       +2     
  Lines       10063    10108      +45     
==========================================
+ Hits         8651     8680      +29     
- Misses       1091     1103      +12     
- Partials      321      325       +4

Flag	Coverage Δ
#integration	`71.09% <ø> (ø)`
#unit	`85.69% <90.66%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
extension/observer/hostobserver/extension.go	`88.88% <88.88%> (ø)`
extension/observer/endpoints.go	`95.34% <100.00%> (+1.59%)`	⬆️
extension/observer/endpointswatcher.go	`94.87% <100.00%> (ø)`
extension/observer/hostobserver/factory.go	`100.00% <100.00%> (ø)`
exporter/elasticexporter/factory.go	`83.33% <0.00%> (-16.67%)`	⬇️
receiver/sapmreceiver/factory.go	`94.11% <0.00%> (-5.89%)`	⬇️
exporter/kinesisexporter/factory.go	`39.58% <0.00%> (-5.32%)`	⬇️
receiver/sapmreceiver/trace_receiver.go	`76.40% <0.00%> (-0.27%)`	⬇️
exporter/sapmexporter/factory.go	`100.00% <0.00%> (ø)`
processor/k8sprocessor/factory.go	`100.00% <0.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fe8114...b3c292f. Read the comment docs.

extension/observer/hostobserver/README.md

keitwb · 2020-07-17T18:31:45Z

extension/observer/hostobserver/README.md

+| port      | port number                                                                                |
+| command   | full command used to invoke this process, including the executable itself at the beginning |
+| is_ipv6   | `true` if the endpoint is IPv6                                                             |
+| protocol  | "TCP" or "UDP"                                                                             |


This is really the transport protocol and IPv4 vs v6 is the network protocol. That distinction was not made very clear in the Smart Agent host observer.

Would you rather have endpoint variables called transport_protocol and network_protocol instead of protocol and is_ipv6?

Discussed with @jrcamp as well and we were thinking of updating these variables to transport_protocol (either TCP or UDP) and network_protocol (either ipv4 or ipv6). What do you think @keitwb ?

ptal @jrcamp @keitwb - #473

extension/observer/hostobserver/extension.go

keitwb · 2020-07-17T18:38:58Z

extension/observer/hostobserver/extension.go

+		logger: logger,
+		EndpointsWatcher: observer.EndpointsWatcher{
+			RefreshInterval: config.RefreshInterval,
+			ListEndpoints:   (&hostObserver{}).listEndpoints,


This is pretty ugly creating a completely separate instance of the host observer to pass the listEndpoints method to. Why don't you just make listEndpoints a plain function instead?

Also logger would be nil in listEndpoints which is not going to behave properly.

Will refactor this and also make sure there's test coverage around this!

extension/observer/hostobserver/README.md

jrcamp · 2020-07-17T20:32:22Z

extension/observer/hostobserver/extension.go

+		logger: logger,
+		EndpointsWatcher: observer.EndpointsWatcher{
+			RefreshInterval: config.RefreshInterval,
+			ListEndpoints:   (&hostObserver{}).listEndpoints,


I wonder if ListEndpoints should be an interface instead of a function. I think it'd look a little cleaner to use since you'd pass in &hostObserver{}. Not critical but see what you think.

extension/observer/hostobserver/extension.go

This PR takes over from #432 - Added SamplingManager to the `jReceiver` struct - Added new parameters to configure remote sampling endpoint (`remote_sampling:fetch_endpoint`) - Added tests and updated README

rmfitzpatrick · 2020-07-23T18:17:08Z

go.mod

@@ -7,34 +7,35 @@ require (
 	github.com/golangci/golangci-lint v1.28.3
 	github.com/google/addlicense v0.0.0-20200622132530-df58acafd6d5
 	github.com/jstemmer/go-junit-report v0.9.1
-	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/alibabacloudlogserviceexporter v0.0.0-00010101000000-000000000000


Were these deviations from https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/487/files#diff-37aff102a57d3d7b797f152915a6dc16 intentional?

@bogdandrutu do you know why these were all changed ^

Yes, these were intentional. Typically we just have the version be v0.0.0 for modules in this repo. Not sure why it was updated. But @bogdandrutu 's changes are as desired, I'll leave it unchanged in this diff.

jrcamp · 2020-07-23T20:45:13Z

extension/observer/endpointswatcher_test.go

-var endpointsMap map[EndpointID]Endpoint
+func setup() (*mockEndpointsLister, EndpointsWatcher, mockNotifier) {
+	ml := &mockEndpointsLister{
+		Mutex:        sync.Mutex{},


Suggested change

Mutex: sync.Mutex{},

Explicit initialization not needed.

jrcamp · 2020-07-23T20:50:53Z

extension/observer/hostobserver/extension.go

+		// still do discovery rules on such sockets.
+		if c.Pid == 0 {
+			cd := collectConnectionDetails(c)
+			id := observer.EndpointID(fmt.Sprintf("%s-%d-%s", c.Laddr.IP, cd.port, cd.transport))


I think we want to include the name of the component instance here. See:

opentelemetry-collector-contrib/extension/observer/k8sobserver/handler.go

Line 48 in 102cde6

podID := observer.EndpointID(fmt.Sprintf("%s/%s", h.idNamespace, pod.UID))

to ensure we don't have id collision across two different host observer instances that discover the same endpoints.

It may not cause collisions for host observer but maybe better to keep consistent.

jrcamp · 2020-07-23T20:52:36Z

extension/observer/hostobserver/extension.go

+
+		pd, err := collectProcessDetails(proc)
+		if err != nil {
+			e.logger.Error(


nit: this doesn't look terribly long if it was just all one one line.

jrcamp · 2020-07-23T20:54:01Z

extension/observer/hostobserver/extension.go

+		pd, err := collectProcessDetails(proc)
+		if err != nil {
+			e.logger.Error(
+				"Skipping process",


Maybe "failed collecting process details (skipping)"

jrcamp · 2020-07-23T20:55:47Z

extension/observer/endpoints.go

@@ -50,6 +50,15 @@ type Port struct {
 	Transport Transport
 }

+// HostPort is an endpoint discovered on a host.
+type HostPort struct {
+	Name      string


What is name for? Looks like below it's set to the ID but is that useful? Please add comment to each one like Pod (though looks like I didn't add them to Port, oops)

Added descriptions to both, fields in HostPort and Port.

jrcamp · 2020-07-23T21:00:28Z

extension/observer/hostobserver/extension.go

+		}
+
+		for _, c := range conns {
+			cd := collectConnectionDetails(*c)


If it's already a pointer probably better to just pass as a pointer instead of making a copy.

jrcamp · 2020-07-23T22:31:45Z

extension/observer/endpoints.go

+	// Port, this value is set by the observer to be "IP-Port-Transport"
+	// prefixed by the name of the host_observer instance,
+	// "(host_observer/2)127.0.0.1-9099-UDP" for example.
+	Name string


If we can't collect it should we just make name be empty string? Why set it to id?

That sounds good. Will update.

jrcamp · 2020-07-23T22:34:46Z

extension/observer/hostobserver/extension.go

+			cd := collectConnectionDetails(&c)
+			id := observer.EndpointID(
+				fmt.Sprintf(
+					"(%s)%s-%d-%s", e.observerName, c.Laddr.IP, cd.port, cd.transport,


I'd like to make it and k8sobserver id format similar but there's some tweaks I want to make to id so I'll just do that separately in another PR.

I think it's better if we use metadata on the end of the ID, similar to what receiver_creator does:

opentelemetry-collector-contrib/receiver/receivercreator/runner.go

Line 101 in bb81db7

// Sets dynamically created receiver to something like receiver_creator/1/redis{endpoint="localhost:6380"}.

So this would look something like:

hostobserver/2{endpoint="127.0.0.1:23", protocol="UDP", pid=3482}

or something like that.

(Just as an aside RC should probably just use the endpoint ID to resolve that TODO)

tl;dr: nothing for you to change here right now, just notes on things to fix sometime.

jrcamp

Just noticed, look like host observer test failed due to races.

asuresh4 · 2020-07-24T00:55:38Z

Just noticed, look like host observer test failed due to races.

Looks like the race was from a couple of tests where I tried to mock failures of third-party packages. Removing those tests for now. Let me know if you feel strongly about adding those back.

asuresh4 · 2020-07-24T16:26:31Z

make[2]: Entering directory '/home/circleci/project/receiver/signalfxreceiver'
running go unit test ./... + coverage in /home/circleci/project/receiver/signalfxreceiver
--- FAIL: Test_signalfxeceiver_EndToEnd (0.01s)
    receiver_test.go:142: 
        	Error Trace:	receiver_test.go:142
        	Error:      	Received unexpected error:
        	            	Post "http://127.0.0.1:45585/v2/datapoint": dial tcp 127.0.0.1:45585: connect: connection refused
        	Test:       	Test_signalfxeceiver_EndToEnd
FAIL
coverage: 95.8% of statements
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/signalfxreceiver	0.578s

Test failure seems unrelated to this PR, will try a rebase to re-trigger tests.

asuresh4 · 2020-07-24T16:45:10Z

ptal @jrcamp

jrcamp · 2020-07-24T17:22:30Z

extension/observer/hostobserver/extension.go

+	}, nil
+}
+
+var getProcessName = func(proc *process.Process) (string, error) {


Maybe in followup PR you could try refactoring this into a struct and/or interface that can be injected by a mock implementation for tests. Should be safer than trying to modify package level variables in test. Can talk about it more offline if it's not clear what I mean.

I think understand what you mean. Will make a subsequent PR to refactor.

bogdandrutu · 2020-07-24T20:48:21Z

Please rebase

asuresh4 · 2020-07-24T21:29:01Z

@bogdandrutu done - ready to merge when you are.

Rename the package from "export" to "metric". Note that all the existing imports of this package use an explicit name of `export` and, therefore, no import declaration changes are included. Rename the `MetricKind` to `Kind` to not stutter in the type usage. Note this does not include a method name change for the `Descriptor` method `MetricKind`.

asuresh4 requested a review from a team as a code owner July 13, 2020 22:43

project-bot bot added this to In progress in Collector Jul 13, 2020

asuresh4 marked this pull request as draft July 13, 2020 22:43

asuresh4 force-pushed the host-observer branch from 5ebd853 to 4d751ce Compare July 14, 2020 15:25

asuresh4 force-pushed the host-observer branch 3 times, most recently from 2a0ea24 to eabd8d4 Compare July 15, 2020 23:00

asuresh4 marked this pull request as ready for review July 16, 2020 14:10

keitwb suggested changes Jul 17, 2020

View reviewed changes

Collector automation moved this from In progress to Review in progress Jul 17, 2020

jrcamp reviewed Jul 17, 2020

View reviewed changes

asuresh4 mentioned this pull request Jul 20, 2020

observer extension: Rename protocol to transport_protocol #473

Merged

asuresh4 force-pushed the host-observer branch 2 times, most recently from 976a3d0 to 0f3be44 Compare July 20, 2020 17:14

jrcamp self-assigned this Jul 20, 2020

asuresh4 force-pushed the host-observer branch from 6242a80 to cb6282f Compare July 21, 2020 17:15

asuresh4 requested review from jrcamp and keitwb July 21, 2020 21:07

jrcamp added this to the Beta 0.7.0 milestone Jul 21, 2020

asuresh4 force-pushed the host-observer branch from 04e7f51 to 88015dc Compare July 22, 2020 16:35

rmfitzpatrick reviewed Jul 23, 2020

View reviewed changes

jrcamp reviewed Jul 23, 2020

View reviewed changes

asuresh4 requested a review from jrcamp July 23, 2020 22:23

jrcamp reviewed Jul 23, 2020

View reviewed changes

Collector automation moved this from Review in progress to Reviewer approved Jul 23, 2020

jrcamp approved these changes Jul 23, 2020

View reviewed changes

jrcamp suggested changes Jul 24, 2020

View reviewed changes

Collector automation moved this from Reviewer approved to Review in progress Jul 24, 2020

asuresh4 requested a review from jrcamp July 24, 2020 16:09

asuresh4 added 4 commits July 24, 2020 12:30

Add host_observer extension

5c44c9b

Address feedback

2187aba

Dont set HostPort.Name if unavailable

14a3311

Remove tests causing race

f0c4f6a

asuresh4 force-pushed the host-observer branch from 5b98ebe to f0c4f6a Compare July 24, 2020 16:31

jrcamp approved these changes Jul 24, 2020

View reviewed changes

Collector automation moved this from Review in progress to Reviewer approved Jul 24, 2020

jrcamp reviewed Jul 24, 2020

View reviewed changes

Merge branch 'master' into host-observer

b3c292f

tigrannajaryan merged commit 158e70c into open-telemetry:master Jul 28, 2020

Collector automation moved this from Reviewer approved to Done Jul 28, 2020

asuresh4 deleted the host-observer branch January 19, 2021 17:03

bogdandrutu pushed a commit that referenced this pull request May 12, 2022

Update CHANGELOG.md (#432)

bb517a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Host Observer #432

Add Host Observer #432

asuresh4 commented Jul 13, 2020 •

edited

codecov bot commented Jul 14, 2020 •

edited

keitwb Jul 17, 2020

asuresh4 Jul 17, 2020

asuresh4 Jul 17, 2020

asuresh4 Jul 20, 2020

keitwb Jul 17, 2020

keitwb Jul 17, 2020

asuresh4 Jul 17, 2020

jrcamp Jul 17, 2020

rmfitzpatrick Jul 23, 2020

jrcamp Jul 23, 2020

asuresh4 Jul 23, 2020

jrcamp Jul 23, 2020

jrcamp Jul 23, 2020

jrcamp Jul 23, 2020

jrcamp Jul 23, 2020

jrcamp Jul 23, 2020

asuresh4 Jul 23, 2020

jrcamp Jul 23, 2020

jrcamp Jul 23, 2020

asuresh4 Jul 23, 2020

jrcamp Jul 23, 2020

jrcamp left a comment

asuresh4 commented Jul 24, 2020

asuresh4 commented Jul 24, 2020 •

edited

asuresh4 commented Jul 24, 2020

jrcamp Jul 24, 2020

asuresh4 Jul 24, 2020

bogdandrutu commented Jul 24, 2020

asuresh4 commented Jul 24, 2020

Add Host Observer #432

Add Host Observer #432

Conversation

asuresh4 commented Jul 13, 2020 • edited

codecov bot commented Jul 14, 2020 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrcamp left a comment

Choose a reason for hiding this comment

asuresh4 commented Jul 24, 2020

asuresh4 commented Jul 24, 2020 • edited

asuresh4 commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Jul 24, 2020

asuresh4 commented Jul 24, 2020

asuresh4 commented Jul 13, 2020 •

edited

codecov bot commented Jul 14, 2020 •

edited

asuresh4 commented Jul 24, 2020 •

edited