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

USM: Expose configuration options of USM #1705

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

guyarb
Copy link
Contributor

@guyarb guyarb commented Feb 19, 2025

What this PR does / why we need it:

Add multiple configuration options of USM, to allow easy configuration for customer.

Default values are intentionally kept as nil, to allow the using the agent's default values unless the customer explicitly modified the option.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer:

Testing:

Use agent default values
datadog:
  serviceMonitoring:
    enabled: true
    httpMonitoringEnabled:
    http2MonitoringEnabled:
    tls:
      go:
        enabled:
      istio:
        enabled:
      nodejs:
        enabled:
      native:
        enabled:

Install helm install datadog-agent -f datadog-values.yaml

Run k exec -it -n datadog-agent -c system-probe daemonset.apps/datadog-agent -- system-probe config show

Expect to see

service_monitoring_config:
  enable_http_monitoring: true
  enable_http2_monitoring: false
  enabled: true
  tls:
    go:
      enabled: true
    istio:
      enabled: true
    native:
      enabled: true
    nodejs:
      enabled: false
Enable everything
datadog:
  serviceMonitoring:
    enabled: true
    httpMonitoringEnabled: true
    http2MonitoringEnabled: true
    tls:
      go:
        enabled: true
      istio:
        enabled: true
      nodejs:
        enabled: true
      native:
        enabled: true

Install helm install datadog-agent -f datadog-values.yaml

Run k exec -it -n datadog-agent -c system-probe daemonset.apps/datadog-agent -- system-probe config show

Expect to see

service_monitoring_config:
  enable_http_monitoring: true
  enable_http2_monitoring: true
  enabled: true
  tls:
    go:
      enabled: true
    istio:
      enabled: true
    native:
      enabled: true
    nodejs:
      enabled: true
Disable everything
datadog:
  serviceMonitoring:
    enabled: true
    httpMonitoringEnabled: false
    http2MonitoringEnabled: false
    tls:
      go:
        enabled: false
      istio:
        enabled: false
      nodejs:
        enabled: false
      native:
        enabled: false

Install helm install datadog-agent -f datadog-values.yaml

Run k exec -it -n datadog-agent -c system-probe daemonset.apps/datadog-agent -- system-probe config show

Expect to see

service_monitoring_config:
  enable_http_monitoring: false
  enable_http2_monitoring: false
  enabled: true
  tls:
    go:
      enabled: false
    istio:
      enabled: false
    native:
      enabled: false
    nodejs:
      enabled: false

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Chart Version bumped
  • Documentation has been updated with helm-docs (run: .github/helm-docs.sh)
  • CHANGELOG.md has been updated
  • Variables are documented in the README.md
  • For Datadog Operator chart or value changes update the test baselines (run: make update-test-baselines)

@guyarb guyarb requested review from a team as code owners February 19, 2025 06:23
@github-actions github-actions bot added the chart/datadog This issue or pull request is related to the datadog chart label Feb 19, 2025
@guyarb guyarb force-pushed the arbitman/USMON-1031 branch from 1363f1b to 0f8fc6b Compare February 19, 2025 07:05
@guyarb guyarb changed the title Arbitman/usmon 1031 USM: Expose configuration options of USM Feb 19, 2025
@guyarb guyarb force-pushed the arbitman/USMON-1031 branch from 0f8fc6b to d059ca4 Compare February 19, 2025 14:45
enable_http2_monitoring: {{ $.Values.datadog.serviceMonitoring.http2MonitoringEnabled }}
{{- end }}
tls:
{{- if not (eq .Values.datadog.serviceMonitoring.tls.go.enabled nil) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we need to use the default values in the agent? It just seems a little odd setting bool value defaults as nil. It seems like this has only been done once in the helm chart before.

Can we just set the defaults we want in values.yaml and avoid all the conditional logic and defaulting booleans to nil? Don't know if your approach is wrong per say, just seems strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason we need to use the default values in the agent?

Yes, some features have been enabled by default over time.

  1. We don’t want the Helm chart to be the source of truth for determining which features are enabled by default.
  2. The Helm chart version and the agent version are independent—updating one does not necessarily mean using the latest of the other. Therefore, we prefer to rely on the agent's version.
  3. Our goal is to provide a simple experience for customers while still allowing them to enable additional features or disable defaults as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart/datadog This issue or pull request is related to the datadog chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants