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

[AIT-8952] Generate DD_INSTRUMENTATION_INSTALL_TIME and DD_INSTRUMENTATION_INSTALL_ID #1263

Merged
merged 15 commits into from
Jan 12, 2024

Conversation

liliyadd
Copy link
Contributor

@liliyadd liliyadd commented Dec 5, 2023

Generates DD_INSTRUMENTATION_INSTALL_TIME and DD_INSTRUMENTATION_INSTALL_ID on installation of Cluster Agent. These env variables are used to power APM time-to-value KPI

https://datadoghq.atlassian.net/browse/AIT-8952

Special notes for your reviewer:

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)

@liliyadd liliyadd marked this pull request as ready for review December 8, 2023 15:37
@liliyadd liliyadd requested a review from a team as a code owner December 8, 2023 15:37
@liliyadd liliyadd requested a review from a team December 8, 2023 15:37
@liliyadd liliyadd changed the title [APM Onboarding] Generate DD_INSTRUMENTATION_INSTALL_TIME and DD_INSTRUMENTATION_INSTALL_ID [AIT-8952] Generate DD_INSTRUMENTATION_INSTALL_TIME and DD_INSTRUMENTATION_INSTALL_ID Dec 8, 2023
{{ include "datadog.labels" . | indent 4 }}
data:
install_id: {{ uuidv4 | quote }}
install_type: unknown
Copy link
Contributor

@CharlyF CharlyF Jan 8, 2024

Choose a reason for hiding this comment

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

is this desired? (from the doc, why is it not k8s_single_step ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected. The default value should be k8s_manual, the same as in Operator.

apiVersion: v1
kind: ConfigMap
metadata:
name: kpi-telemetry-configmap
Copy link
Collaborator

@clamoriniere clamoriniere Jan 8, 2024

Choose a reason for hiding this comment

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

I'm not sure if it is good to have a fix name here. because we have some use case (like windows install that require to deploy/install twice this chart in the same namespace)

Also can we create this configmap only if APM SSI is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Parametrized the Configmap name with .Release.name
  • We need this env variables to get KPIs for all of manual instrumentation, local library injection and SSI. Thus we cannot condition configmap on SSI enablement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok but maybe at least when the admission controller is enabled

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was too quick merging this. This is not in parity with the Operator, which adds these envs only if APM feature is enabled. If we add AC check here, we should update Operator code, otherwise we add APM check here.

Copy link
Contributor Author

@liliyadd liliyadd Jan 12, 2024

Choose a reason for hiding this comment

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

This is not in parity with the Operator, which adds these envs only if APM feature is enabled

This is the Operator code that sets these env variables. I believe we always set these env variables and not only when APM is enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to trace agent, which is not run when APM is disabled so behavior should match. Sorry for confusion.

@liliyadd liliyadd requested review from clamoriniere, CharlyF and a team January 10, 2024 15:55
@@ -151,3 +151,13 @@ Return a list of env-vars if the cluster-agent is enabled
key: token
{{- end }}
{{- end -}}


{{- define "kpi-envvar" -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this redundant since env vars are inlined in both trace agent and DCA containers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching it. This code re-appeared after merge from remote branch.

@liliyadd liliyadd requested a review from levan-m January 12, 2024 18:53
@levan-m levan-m merged commit eafa471 into main Jan 12, 2024
14 checks passed
@levan-m levan-m deleted the liliya.belaus/apm-kpi-envvars branch January 12, 2024 20:09
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.

None yet

4 participants