[resourcedetectionprocessor] Add docker detector #2775

mx-psi · 2021-03-19T16:06:02Z

Description:

Add Docker detector

Link to tracking Issue: Fixes #2773

Testing: Amended unit tests. Tested on Ubuntu 18.04 (you need to add -v /var/run/docker.sock:/var/run/docker.sock).

Documentation: Documented on README

codecov · 2021-03-19T16:54:15Z

Codecov Report

Merging #2775 (ebc2ed6) into main (80eb4fc) will decrease coverage by 0.04%.
The diff coverage is 60.60%.

@@            Coverage Diff             @@
##             main    #2775      +/-   ##
==========================================
- Coverage   91.61%   91.56%   -0.05%     
==========================================
  Files         486      488       +2     
  Lines       23540    23568      +28     
==========================================
+ Hits        21565    21581      +16     
- Misses       1465     1473       +8     
- Partials      510      514       +4

Flag	Coverage Δ
integration	`63.09% <ø> (-0.27%)`	⬇️
unit	`90.57% <60.60%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
...urcedetectionprocessor/internal/system/metadata.go	`0.00% <0.00%> (-57.15%)`	⬇️
...sourcedetectionprocessor/internal/docker/docker.go	`46.66% <46.66%> (ø)`
...urcedetectionprocessor/internal/docker/metadata.go	`69.23% <69.23%> (ø)`
...cedetectionprocessor/internal/resourcedetection.go	`97.22% <100.00%> (+0.16%)`	⬆️

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 80eb4fc...ebc2ed6. Read the comment docs.

github-actions · 2021-03-27T05:21:08Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

mx-psi · 2021-03-29T16:00:27Z

@kbrockhoff can you take a look? Happy to try to improve test coverage after I have a review on the overall idea

github-actions · 2021-04-06T05:21:20Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

mx-psi · 2021-04-16T13:33:37Z

@kbrockhoff Friendly ping!

Also @anuraaga @jrcamp I would be happy if you took a look :)

bogdandrutu · 2021-04-16T17:22:13Z

@jrcamp please review :)

jrcamp · 2021-04-19T16:23:41Z

processor/resourcedetectionprocessor/README.md

@@ -17,6 +17,8 @@ details of which are currently pending confirmation in the OpenTelemetry specifi
    * host.name
    * os.type

+    If running the Collector Docker image, mount `/var/run/docker.sock` and set the `use_docker_sock` flag to `true` to retrieve the attributes from the host machine. Docker detection does not work on macOS.


I kinda feel like it should be its own detector (you called it out as a possibility in the original issue). Was there any reason you decided not to go that route?

Was there any reason you decided not to go that route?

The main reasons were that

it felt like the Docker/bare system detectors should evolve together and provide the same attributes when technically feasible: we get the same attributes today and if e.g. someone wants to add the host.arch attribute when running the Collector binary it makes sense that they would also want it when running on Docker.

the system detector should work regardless of how the Collector is ran: from a user's POV it should make no difference if they are running the Collector from a bare binary or a containerized version.

But as I wrote, I can definitely see it being its own detector, specially if we think they might end up diverging in some way (e.g. we want to add a docker.version attribute or some other attribute that only makes sense on Docker). We have to have a use_docker_sock flag anyway so it's not going to make the user experience much worse and it is more flexible

@jrcamp I thought about this a bit more, and while I think the arguments above make sense, I think it's likely that these detector end up diverging so I separated it, PTAL

github-actions · 2021-04-27T05:21:04Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

tigrannajaryan · 2021-04-30T17:07:03Z

@anuraaga @jrcamp please review.

anuraaga · 2021-05-06T07:30:34Z

processor/resourcedetectionprocessor/README.md

+
+* Docker metadata: Queries the Docker daemon to retrieve the following resource attributes from the host machine:
+
+    * host.name


Can you describe the difference between retrieving these with normal means? I'm surprised we need to use the docker API to get simple looking attributes.

Maybe it's to retrieve the information about the host, not the docker container. If so, this seems wrong, I believe our semantic conventions intend these to be filled for the current process, which would be using the values from within the docker container, not the host machine. I'd expect us to define new conventions in the spec, something like containerhost.host.name, containerhost.os.type for things outside the process.

Sorry if I'm missing something

Also I was expecting this PR to implement container attributes

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/container.md#container

Surprised to see host.name and os.type in the docker detector :)

Can you describe the difference between retrieving these with normal means? I'm surprised we need to use the docker API to get simple looking attributes.

Here is the output of the hostname command and the Name field from the Docker API info endpoint from an Ubuntu Docker image running on an Ubuntu VM:

❯ docker run --rm -v /var/run/docker.sock:/var/run/docker.sock -it ubuntu bash root@3d639eae3af3:/# apt-get update && apt-get install -y curl jq # ... root@3d639eae3af3:/# hostname 3d639eae3af3 root@3d639eae3af3:/# curl --silent --unix-socket /var/run/docker.sock http://localhost/info | jq .Name "ubuntu-vm"

hostname gives you the os.Hostname (which is a random string on Docker). The Docker API gives you the name of the host machine (on Linux and Windows).

Similarly, the os.type will give you the host OS type (so, e.g., windows even if your container is Linux-based).

I'd expect us to define new conventions in the spec, something like containerhost.host.name, containerhost.os.type for things outside the process.

I can definitely do that, but I fail to see why someone would care about the Docker hostname: the whole point (at least to me) of running the Collector as a Docker image is to not have other stuff running in the same Docker image so the useful info is the one about the host where the Docker image is running.

Also I was expecting this PR to implement container attributes

👍 will do

Ah, thanks for the explanation, that's the opposite behavior of what I was expecting for os.type, surprised native calls within the container don't return information about the container :O And populating something other than the random string seems good.

Just curious, do you know how common it is to have the docker API exposed? For example, will kubernetes users generally be able to use the detector?

Just curious, do you know how common it is to have the docker API exposed? For example, will kubernetes users generally be able to use the detector?

I don't know about Kubernetes specifically, I would expect that people have to mount the Unix socket manually. We use this on our Datadog Agent for users running Docker by itself, but we have a different solution for Kubernetes.

anuraaga

I think we can add further container attributes in a future PR now I understand that this is important for correctly populating those existing ones

anuraaga · 2021-05-06T23:49:58Z

processor/resourcedetectionprocessor/README.md

+
+* Docker metadata: Queries the Docker daemon to retrieve the following resource attributes from the host machine:
+
+    * host.name


Ah, thanks for the explanation, that's the opposite behavior of what I was expecting for os.type, surprised native calls within the container don't return information about the container :O And populating something other than the random string seems good.

Just curious, do you know how common it is to have the docker API exposed? For example, will kubernetes users generally be able to use the detector?

bogdandrutu · 2021-05-12T17:26:17Z

Please rebase, codecov should not run :)

bogdandrutu · 2021-05-13T16:28:30Z

@jrcamp if you have any other comment please open an issue.

mx-psi · 2021-05-14T08:29:57Z

Sorry, I was out yesterday and didn't see this, happy to see there was no problem with the merge

* Add a host wrapper to improve ReportFatalError msg * Adding host wrapper when starting pipeline * Do not allocate in the loop

mx-psi added 3 commits March 19, 2021 17:04

Detect host.name and os.type on system detector when running on Docker

c941ad5

s/auto/false

49e42e9

Fix unit test

6fa036d

mx-psi marked this pull request as ready for review March 19, 2021 17:12

mx-psi requested review from anuraaga and jrcamp as code owners March 19, 2021 17:12

mx-psi requested a review from a team March 19, 2021 17:12

github-actions bot assigned kbrockhoff Mar 19, 2021

github-actions bot added the Stale label Mar 27, 2021

Merge branch 'main' into mx-psi/system-docker

bba20a1

github-actions bot removed the Stale label Mar 30, 2021

github-actions bot added the Stale label Apr 6, 2021

bogdandrutu removed the Stale label Apr 12, 2021

mx-psi added 2 commits April 16, 2021 13:58

Merge branch 'main' into mx-psi/system-docker

b3c87a3

Add some tests for Docker provider

1f15a0f

jrcamp assigned jrcamp and unassigned kbrockhoff Apr 19, 2021

jrcamp reviewed Apr 19, 2021

View reviewed changes

github-actions bot added the Stale label Apr 27, 2021

mx-psi added 2 commits April 27, 2021 13:00

Move to a separate detector

c1024b9

Remove unnecessary changes

ebc2ed6

mx-psi changed the title ~~Detect host.name and os.type on system detector when running on Docker~~ [resourcedetectionprocessor] Add docker detector Apr 27, 2021

mx-psi requested a review from jrcamp April 27, 2021 11:07

tigrannajaryan removed the Stale label Apr 27, 2021

anuraaga reviewed May 6, 2021

View reviewed changes

anuraaga approved these changes May 6, 2021

View reviewed changes

bogdandrutu merged commit a7a9ead into open-telemetry:main May 13, 2021

mx-psi deleted the mx-psi/system-docker branch May 14, 2021 07:33

alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021

Add a host wrapper to improve ReportFatalError msg (#2775)

c81a01b

* Add a host wrapper to improve ReportFatalError msg * Adding host wrapper when starting pipeline * Do not allocate in the loop

ChrsMark mentioned this pull request Nov 1, 2024

Additional Docker Container Metadata to [resourcedetection/docker] #36105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[resourcedetectionprocessor] Add docker detector #2775

[resourcedetectionprocessor] Add docker detector #2775

mx-psi commented Mar 19, 2021 •

edited

Loading

codecov bot commented Mar 19, 2021 •

edited

Loading

github-actions bot commented Mar 27, 2021

mx-psi commented Mar 29, 2021

github-actions bot commented Apr 6, 2021

mx-psi commented Apr 16, 2021

bogdandrutu commented Apr 16, 2021

jrcamp Apr 19, 2021

mx-psi Apr 19, 2021

mx-psi Apr 27, 2021

github-actions bot commented Apr 27, 2021

tigrannajaryan commented Apr 30, 2021

anuraaga May 6, 2021

anuraaga May 6, 2021

mx-psi May 6, 2021 •

edited

Loading

anuraaga May 6, 2021

mx-psi May 7, 2021

anuraaga left a comment

anuraaga May 6, 2021

bogdandrutu commented May 12, 2021

bogdandrutu commented May 13, 2021

mx-psi commented May 14, 2021


		* Docker metadata: Queries the Docker daemon to retrieve the following resource attributes from the host machine:

		* host.name

[resourcedetectionprocessor] Add docker detector #2775

[resourcedetectionprocessor] Add docker detector #2775

Conversation

mx-psi commented Mar 19, 2021 • edited Loading

codecov bot commented Mar 19, 2021 • edited Loading

Codecov Report

github-actions bot commented Mar 27, 2021

mx-psi commented Mar 29, 2021

github-actions bot commented Apr 6, 2021

mx-psi commented Apr 16, 2021

bogdandrutu commented Apr 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2021

tigrannajaryan commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented May 12, 2021

bogdandrutu commented May 13, 2021

mx-psi commented May 14, 2021

mx-psi commented Mar 19, 2021 •

edited

Loading

codecov bot commented Mar 19, 2021 •

edited

Loading

mx-psi May 6, 2021 •

edited

Loading