feat: add container resource detector. #340

scbjans · 2023-02-16T15:07:55Z

This PR adds a resource detector for Containers.

It will add the container ID if possible.

~~For containers running on a Docker runtime, it will also try to add~~
* container.image.name
* container.image.tag
* container.name
* container.runtime

~~but that depends on the availability of a readable Docker Socket.~~

I've decided to remove setting additional container resource attributes and only support setting the container.id.

Reasoning behind that:

Setting other container resource attributes requires a readable docker.sock/containerd.sock, which requires correct - probably most of the time equals to elevated - permissions. Most easy way to do that is of course running the container as root. I do not want to promote that.
I could try and add support for configuring access to the docker.sock using TLS certs; but that would make auto-instrumentation a lot less auto
Other languages also only seem to be setting container.id

Fixes: #179

…tector

resource_detectors/lib/opentelemetry/resource/detectors/container.rb

…support for cgroup v1.

mwear

This mostly LGTM. I just had a question about cgroup v2 this time. I'm not requesting changes, but I'd like to clarify that the strategy we're using is at least as reliable as others I've seen.

mwear · 2023-04-04T21:59:06Z

resource_detectors/lib/opentelemetry/resource/detectors/container.rb

+            parts.shift
+
+            parts.each do |part|
+              return part if part.match?(CONTAINER_REGEX)


Other implementations that I have seen specifically look for a line containing "hostname" to extract the container id from. For example:

Java

Javascript

However, looking at your test, and inspecting /proc/self/mountinfo on some containers running locally, it looks like this should work just fine. Do you know of any other implementations that use this strategy? I'm curious if they know something we don't, or if there are multiple solutions that work.

Thanks for your re-review @mwear.

My guess would be that filtering on "hostname" is done to limit the amount of regexmatching?
I initially included the filter on "hostname" as well; but decided to remove that; since it cannot be used reliably. e.g. when a user set the hostname themselves in kubernetes or swarm; we would not be able to determine the container.id (although I think setting the "hostname" will probably be an edge case).

If we decide that we don't want to filter on "hostname" before regex matching; we might also remove container_id_v1; since def container_id_v2 will also work for cgroupv1 - although I still want to benchmark that before doing so - to keep the code a bit simpler. WDYT?

I don't have a strong preference. I am most interested in the detector reliably and consistently detecting the container id. As far as performance goes, detecting the container id will be something that happens once at startup. While we don't want to give performance away unnecessarily, this is not a hot code path, so favoring simplicity is reasonable thing to do here. Let me know when you're happy with everything and I'll review it again.

I've simplified the code a bit; according to me this should do the trick.

mwear

Looks great! Thanks @scbjans.

mwear · 2023-04-13T21:13:42Z

@robertlaurin could we get an additional set of eyes on this when you get a chance?

scbjans · 2023-04-16T18:19:19Z

@mwear @robertlaurin from the SIG I understood that the plan was to split the resource detectors gem into separate gems. That totally makes sense to me; and I'll rework this PR a bit to reflect that. Once that's done and approved, I'm happy to also split of the other two into separate gems.

One point on which I would like advice or some directions is the addition of other container resource attributes (see PR description, and - currently outdated - image). It'll be helpful to add the other container resource attributes as well; but I do not want to promote running the container as root. Do you perhaps have another idea on how I could add them?

arielvalentin · 2023-04-19T19:00:29Z

resource_detectors/lib/opentelemetry/resource/detectors/container.rb

+        # @return [String] container.id
+        #   May be nil.
+        def container_id
+          [CGROUP_V2_PATH, CGROUP_V1_PATH].each do |cgroup|


Would emitting some kind of warning or error message be helpful to folks trying to debug why this resource detector is unable to read the cgroup files?

Originally my idea was that we do not want to flood the logs if someone uses this gems Autodetector.detect, but since we're going to split the resource detectors up into separate gems, that reasoning is no longer valid.

I'll add some logs once I get to splitting this up into it's own gem.

arielvalentin · 2023-04-19T19:01:19Z

resource_detectors/lib/opentelemetry/resource/detectors/container.rb

+        # If no container attributes could be determined an empty resource is returned
+        #
+        # @return [OpenTelemetry::SDK::Resources::Resource]
+        def detect


I am not entirely certain what the spec says about error handling in Resource detectors but I recommend you consider adding error handling with diagnostic information that will clue folks into why this resource detector may be failing and avoid raising errors that would cause the application to crash at boot time.

Will do so once I split this of into it's own separate gem (see comment above). However, I do believe that currently this resource detector will not crash if it cannot determine the container.id?

arielvalentin · 2023-04-19T19:02:49Z

resource_detectors/lib/opentelemetry/resource/detectors/container.rb

+            next unless File.readable?(cgroup)
+
+            File.readlines(cgroup, chomp: true).each do |line|
+              parts = line.split('/')


Is there some nuance I am missing here. Does scanning the full string result in false positives?

scanning the full string works for cgroupv1; but I couldn't get it to work for cgroupv2. This way it's compatible with both

scbjans · 2023-04-20T19:55:27Z

The container.id cannot be determined reliably when the container has additional copy-on-write layers written to it's filesystem. I'll have to sort that out first -> returning to draft mode.

scbjans · 2023-05-09T10:56:50Z

Updated PR to only filter out lines including an actual container id. Previously I ran into problems because storage ids also matched. As a bonus I've included a test so that it should also work for Podman (cgroupfile copied from the java instrumentation).

scbjans · 2023-05-09T10:58:22Z

Regarding adding the other container resource attributes: I've learned that I can use the otelcols k8sattributesprocessor to fix that.

scbjans · 2023-06-01T14:25:31Z

@robertlaurin is this how you envisioned splitting up the resource detectors into it's own seperate gem? If so, I'm happy to do it for the other two resource detectors (Azure and GCP) as well.

arielvalentin · 2023-06-22T14:04:43Z

@mwear @ericmustin anything left here to do before merging?

ericmustin · 2023-06-22T14:13:44Z

@arielvalentin lgtm

arielvalentin · 2023-06-22T16:58:44Z

@ericmustin would you mind merging and releasing?

github-actions · 2023-07-23T01:57:25Z

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

scbjans · 2023-07-25T07:39:26Z

@mwear @ericmustin @arielvalentin anything I can do to get this merged?

…tector

scbjans · 2023-07-28T06:42:36Z

CI is broken; but there's already a PR which fixes that: #581

…tector

scbjans added 5 commits February 16, 2023 15:59

feat: add container resource detector.

a5fc566

add tests.

c9b3570

Merge branch 'open-telemetry:main' into 179-add-container-resource-de…

cc235fd

…tector

remove setting additional container resource attributes.

5c0e82d

remove docker stubs from auto_detector_test.

2e49405

scbjans marked this pull request as ready for review March 27, 2023 11:50

scbjans requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam and robbkidd as code owners March 27, 2023 11:50

mwear reviewed Mar 28, 2023

View reviewed changes

resource_detectors/lib/opentelemetry/resource/detectors/container.rb Outdated Show resolved Hide resolved

scbjans marked this pull request as draft March 29, 2023 07:11

scbjans added 2 commits April 3, 2023 16:25

do not rely on hostname (since it can be changed by enduser) and add …

38bbe8e

…support for cgroup v1.

appease rubocop.

5b15423

scbjans marked this pull request as ready for review April 3, 2023 15:06

mwear reviewed Apr 4, 2023

View reviewed changes

simplify code a bit; add yardoc.

d2cc1be

scbjans requested a review from mwear April 7, 2023 12:31

mwear approved these changes Apr 7, 2023

View reviewed changes

arielvalentin reviewed Apr 19, 2023

View reviewed changes

scbjans marked this pull request as draft April 20, 2023 19:50

container resource detector split into its own gem.

a2ef2a7

scbjans added 3 commits April 20, 2023 22:35

fixes for building. No fix yet for determining container.id reliably.

3818b5c

build fix.

7f39a11

only filter out container IDs

54a6885

scbjans marked this pull request as ready for review May 9, 2023 10:53

ericmustin approved these changes May 9, 2023

View reviewed changes

github-actions bot added the stale Marks an issue/PR stale label Jul 23, 2023

github-actions bot removed the stale Marks an issue/PR stale label Jul 26, 2023

Merge branch 'open-telemetry:main' into 179-add-container-resource-de…

cd5161d

…tector

scbjans added 2 commits July 28, 2023 17:40

Merge branch 'open-telemetry:main' into 179-add-container-resource-de…

ce814c8

…tector

rubocop fix.

c19660a

arielvalentin merged commit 2039d15 into open-telemetry:main Jul 28, 2023
43 checks passed

scbjans deleted the 179-add-container-resource-detector branch July 31, 2023 08:28

scbjans mentioned this pull request Aug 3, 2023

REQUEST: New membership for @scbjans open-telemetry/community#1630

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add container resource detector. #340

feat: add container resource detector. #340

scbjans commented Feb 16, 2023 •

edited

Loading

mwear left a comment

mwear Apr 4, 2023

scbjans Apr 6, 2023

mwear Apr 6, 2023

scbjans Apr 7, 2023

mwear left a comment

mwear commented Apr 13, 2023

scbjans commented Apr 16, 2023

arielvalentin Apr 19, 2023

scbjans Apr 20, 2023

arielvalentin Apr 19, 2023

scbjans Apr 20, 2023

arielvalentin Apr 19, 2023

scbjans Apr 20, 2023

scbjans commented Apr 20, 2023

scbjans commented May 9, 2023

scbjans commented May 9, 2023

scbjans commented Jun 1, 2023

arielvalentin commented Jun 22, 2023

ericmustin commented Jun 22, 2023

arielvalentin commented Jun 22, 2023

github-actions bot commented Jul 23, 2023

scbjans commented Jul 25, 2023

scbjans commented Jul 28, 2023

feat: add container resource detector. #340

feat: add container resource detector. #340

Conversation

scbjans commented Feb 16, 2023 • edited Loading

mwear 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

mwear left a comment

Choose a reason for hiding this comment

mwear commented Apr 13, 2023

scbjans commented Apr 16, 2023

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

scbjans commented Apr 20, 2023

scbjans commented May 9, 2023

scbjans commented May 9, 2023

scbjans commented Jun 1, 2023

arielvalentin commented Jun 22, 2023

ericmustin commented Jun 22, 2023

arielvalentin commented Jun 22, 2023

github-actions bot commented Jul 23, 2023

scbjans commented Jul 25, 2023

scbjans commented Jul 28, 2023

scbjans commented Feb 16, 2023 •

edited

Loading