add container id to ApiServer.State and send in header #38
Conversation
| headers = @headers ++ [{"X-Datadog-Trace-Count", length(traces)}] | ||
| headers = headers ++ List.wrap(if container_id, do: {"Datadog-Container-ID", container_id}) |
There was a problem hiding this comment.
reference to this header in DataDog's Node.js client
https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/src/exporters/agent/request.js#L26
lib/spandex_datadog/api_server.ex
Outdated
| @cgroup_uuid "[0-9a-f]{8}[-_][0-9a-f]{4}[-_][0-9a-f]{4}[-_][0-9a-f]{4}[-_][0-9a-f]{12}" | ||
| @cgroup_ctnr "[0-9a-f]{64}" | ||
| @cgroup_task "[0-9a-f]{32}-\\d+" | ||
| @cgroup_regex Regex.compile!("/(#{@cgroup_uuid}|#{@cgroup_ctnr}|#{@cgroup_task})(?:.scope)?$", "m") |
There was a problem hiding this comment.
reference to this regex used to get the container_id
https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/src/exporters/agent/docker.js
|
adding a link to an issue that references this concern. #25 |
There was a problem hiding this comment.
Hi @dallincrane - thanks for contributing! We could actually use this feature at my company, so I took the liberty to check it. I have just one comment as to the implementation.
When it comes to testing, I have two ideas:
- Inject the
cgroupfile path either through opts, or through application env, and default to/proc/self/cgroup. Then override that value in tests as e.g.test/support/cgroup. - Extract
cgroupparsing to a separate module and expose a function likeparse/1which takes the file path as an argument. Test that module separately, then use it inApiServer. You could even usemoxto mock that parsing module in tests.
Let me know what you think.
|
I can also help to test this - I'll spin up an environment with this change and see if it works. |
|
Seems to be working 👏 |
Nice! At my last company we had tested it and had it in production. Glad you are seeing the same! |
|
Hi @kamilkowalski - thanks for input! Do you by chance have write access? If you don't, it looks like we need someone with write access to provide a review and perhaps give a little input into some of your suggestions. For some background, I was trying to choose code that provided the least resistance and least amount of project structure change so it could be accepted and quickly released.
I used Datadog's Javascript code as a refernence. It is hard coded there. I'm not sure how many projects would have a custom cgroup file path
Generally I do really like what you are suggesting and it even matches the Datadog code! But because adding a new file and module requires a naming discussion and |
|
@dallincrane I'm not a maintainer so we probably need to wait for @zachdaniel or @GregMefford to chip in. As to testing - I think sometimes it makes sense to extend the library's public interface to allow for testing (if the alternative is no tests). Also, the second solution doesn't necessarily require Mox - it only requires us to extract the cgroups functionality to a separate module. |
|
FYI: regex used in the reference implementation has been changed since this PR was opened: DataDog/dd-trace-js#1314. |
|
thank you @moleskin-smile! updated |
|
Hey there! Thanks again for this contribution! I finally got a chance to try it out and verified that it's working, so I will merge now and work on releasing an update version that include this. ❤️ 🚀 |
Adds a
Datadog-Container-IDheader when a container is being used.Modeled on the DD Node.js client code
Happy to add a couple tests if required but would need some guidance — the file read might require a mock setup.