Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for configuring APM injection #481

Merged
merged 18 commits into from
Jun 27, 2023
Merged

Conversation

rarguelloF
Copy link
Contributor

@rarguelloF rarguelloF commented May 4, 2023

This PR adds support for configuring APM injection for a host (see: https://docs.datadoghq.com/tracing/trace_collection/library_injection_local/?tab=host#configure-docker-injection).

The supported scenarios are:

  • Agent and applications running on the same host (datadog_apm_instrumentation_enabled: host).
  • Agent and applications running in separate Docker containers on the same host (datadog_apm_instrumentation_enabled: docker).
  • All the previous scenarios at the same time (datadog_apm_instrumentation_enabled: all).

Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋 thanks for sending a PR. I think this is a good start, but it will need some more work. I left comments inline addressing what needs to be done.

defaults/main.yml Outdated Show resolved Hide resolved
tasks/apm-host-injection.yml Outdated Show resolved Hide resolved
tasks/apm-host-injection.yml Outdated Show resolved Hide resolved
tasks/apm-host-injection.yml Outdated Show resolved Hide resolved
tasks/apm-host-injection.yml Outdated Show resolved Hide resolved
tasks/pkg-debian/install-apm-host-inject-latest.yml Outdated Show resolved Hide resolved
tasks/pkg-redhat/install-apm-host-inject-latest.yml Outdated Show resolved Hide resolved
tasks/apm-host-injection.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
@rarguelloF rarguelloF requested a review from bkabrda May 30, 2023 10:19
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋 thanks for the additional work, this looks much better now! I left couple comments to address inline. I would also like to ask you to add the descriptions of all newly added configuration values to the README.md file.

tasks/pkg-debian/install-apm-inject-latest.yml Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
tasks/pkg-redhat/install-apm-inject-latest.yml Outdated Show resolved Hide resolved
tasks/pkg-redhat/install-apm-inject-latest.yml Outdated Show resolved Hide resolved
tasks/apm-inject-check.yml Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
tasks/apm-inject-check.yml Show resolved Hide resolved
tasks/apm-inject-install.yml Outdated Show resolved Hide resolved
tasks/apm-inject-install.yml Show resolved Hide resolved
tasks/apm-inject-install.yml Outdated Show resolved Hide resolved
tasks/apm-inject-install.yml Outdated Show resolved Hide resolved
@rarguelloF rarguelloF marked this pull request as ready for review June 16, 2023 16:31
@rarguelloF rarguelloF requested review from a team as code owners June 16, 2023 16:31
@rarguelloF rarguelloF changed the title WIP: add support for installing APM injection Add support for configuring APM injection Jun 16, 2023
@rarguelloF rarguelloF requested a review from bkabrda June 16, 2023 16:31
Copy link
Contributor

@hestonhoffman hestonhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small edit

defaults/main.yml Outdated Show resolved Hide resolved
Co-authored-by: Heston Hoffman <[email protected]>
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋 this now looks almost ready, but I still found some problems that should be addressed before I can approve.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tasks/pkg-debian/install-apm-inject-latest.yml Outdated Show resolved Hide resolved
tasks/apm-inject-check.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, great job!

@bkabrda bkabrda merged commit 16e6af9 into main Jun 27, 2023
@bkabrda bkabrda deleted the rarguelloF/apm-injection branch June 27, 2023 12:42
@maxiangelo
Copy link

Hi didnt you forgot the Agent on host, app in containers possibillity of injecting APM?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants