-
Notifications
You must be signed in to change notification settings - Fork 1k
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 helm chart installation for OTel Agent #1429
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a quick first review
charts/datadog/values.yaml
Outdated
- containerPort: 4317 | ||
hostPort: 4317 | ||
- containerPort: 4318 | ||
hostPort: 4318 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think network policies need also to be updated. to expose the port.
and hostPort
doesn't need to be set by default. see how it is done for the agent container
helm-charts/charts/datadog/templates/_container-agent.yaml
Lines 35 to 37 in 036f328
{{- if .http.useHostPort }} | |
hostPort: {{ .http.endpoint | regexFind ":[0-9]+$" | trimPrefix ":" }} | |
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dineshg13
the changes present seems to be ok.
However some changes are missing:
- update the NetworkPolicy and Cilium Network Policy to allow the traffic on the new port.
- Add a test cas in https://github.com/DataDog/helm-charts/tree/main/charts/datadog/ci to test that the the generated manifests are valid and the agent start properly
Also I'm wondering if the work to be supported in GKE autopilot has been done already with the @DataDog/container-ecosystems team to define the new profil. If not maybe, the best for not is to disable OTEL container if GKE autopilot support is enable.
I let the container-ecosystem team responsible of the datadog
chart the end of this PR review.
last, If you can add in the PR description more information about how to test it. For example the minimum values.yaml
needed to install the chart with the OTEL container. 🙇
{{- if .Values.providers.gke.autopilot -}} | ||
datadog-agent-otel-config | ||
{{- else -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DataDog/container-ecosystems have you been able to update the Agent profile in GKE autopilot to support the additional OTEL container?
@clamoriniere how do i update Celium/Network policy ? How can we handle the case where customer might use a different port for OTLP receiver ? |
agents: | ||
image: | ||
repository: datadog/agent-dev | ||
tag: nightly-ot-main | ||
doNotCheckTag: true | ||
containers: | ||
agent: | ||
env: | ||
- name: DD_OTELCOLLECTOR_ENABLED | ||
value: "true" | ||
- name: DD_HOSTNAME | ||
value: "datadog" | ||
datadog: | ||
apiKey: "00000000000000000000000000000000" | ||
appKey: "0000000000000000000000000000000000000000" | ||
otelCollector: | ||
enabled: true | ||
config: | | ||
receivers: | ||
otlp: | ||
exporters: | ||
datadog: | ||
api: | ||
key: "00000000000000000000000000000000" | ||
service: | ||
pipelines: | ||
traces: | ||
receivers: [otlp] | ||
exporters: [datadog] | ||
metrics: | ||
receivers: [otlp] | ||
exporters: [datadog] | ||
logs: | ||
receivers: [otlp] | ||
exporters: [datadog] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the default configuration shorter for otel support activation. like only to have to set datadog.otelcollector.enabled: true
When I saw the dash demo, it looks like only 2 parameters was needed.
I hope that we can simplify at least like
agents:
image:
tagSuffix: ot-beta
datadog:
apiKey: "00000000000000000000000000000000"
appKey: "0000000000000000000000000000000000000000"
otelCollector:
enabled: true
so my questions are, when datadog.otelcollector.enabled: true
can we:
- generate a default configuration that use the API provide in the
datadog.apiKey
field? - set
agents.image.tagSuffix
withot-beta
? - inject the needed envvars in the core-agent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good feedback. Thanks for same. I have addressed same in the latest commit .
if datadog.otelcollector.enabled: true
we do the following
- set agents.image.tagSuffix with ot-beta if image tag isn't set
- inject the needed envvar
DD_OTELCOLLECTOR_ENABLED
in the core-agent . - if
datadog.otelcollector.config
is not provided, use the default config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only 2 new comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dineshg13
Thank you for considering all my requests for changes. 🙇
I think the result is great the user experience for OTEL user is now good 👍
cheers
/merge |
/merge |
🚂 MergeQueue: waiting for PR to be ready This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
b8ece16
to
8f82022
Compare
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
What this PR does / why we need it:
Adds OTel Agent container . OTel Agent is a Datadog's OpenTelemetry's collector distro that runs along with Agent.
The PR add example collector configuration and corresponding agent configuration.
To test the changes use the below helm command
Special notes for your reviewer:
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
.github/helm-docs.sh
)CHANGELOG.md
has been updatedREADME.md
make update-test-baselines
)