elastic / opentelemetry

Get started with our Elastic Distros of OpenTelemetry
11 stars 8 forks source link

[k8s-otel][docs]Custom configuration and upgrade sections #50

Closed eedugon closed 1 week ago

eedugon commented 2 weeks ago

Added the following sub-sections to Kubernetes onboarding documentation 👍

I'll be requesting in this PR first a review by docs team and then by product team (@rogercoll / @gizas / @mlunadia ).

Closes https://github.com/elastic/opentelemetry/issues/35

eedugon commented 2 weeks ago

@rogercoll , or @gizas : I'd like you to review this PR and let me know if there are other configuration settings you believe we should include for customers to consider customizing when installing the operator.

Should we add or describe the default resources used by the operator for the collectors?

Also, what about this sentence?

The namespace of the installation cannot be changed and must be set to opentelemetry-operator-system during the helm chart installation.

Is it accurate? I've added it because I tried to customize the namespace directly in the helm install command and the installation failed, so I guess we might have the namespace defined somewhere else in the chart.

If we need to support installation in a different namespace we should probably create a separate issue for it.

gizas commented 2 weeks ago

Line 145: $ helm upgrade --install --namespace opentelemetry-operator-system opentelemetry-kube-stack open-telemetry/opentelemetry-kube-stack --values ./resources/kubernetes/operator/helm/values.yaml --version 0.3.0

Please Update the version to --version 0.3.3

eedugon commented 2 weeks ago

thanks @gizas ! I'll apply the suggested changes :)

gizas commented 2 weeks ago

I am little worried with the compatibility matrix that we can forget to update as we upgrade the versions. Can we make sure that we make this update part of DoD somewhere or automation step?

The link Values files are available in the resources directory points to a custom reference, I would say to link it with main

Also in the matrix can you update the 8.16.0 values.yaml to have a link to https://github.com/elastic/opentelemetry/blob/8.16/resources/kubernetes/operator/helm/values.yaml and the chart version to be 0.3.3 !!!

For additional things to describe, I would say for now we are ok. I will let other to comment on this

For the namespace I did a smoke test and worked for me

Commands to install to new namespace kubectl create namespace opentelemetry-operator-system2 kubectl create secret generic elastic-secret-otel --namespace opentelemetry-operator-system2 --from-literal=elastic_endpoint='https://f670e0b954504a56819e3172d9d09af4.us-west2.gcp.elastic-cloud.com:443' --from-literal=elastic_api_key='Yk11ajlEJ4bExEYlBNQQ==' helm repo add open-telemetry 'https://open-telemetry.github.io/opentelemetry-helm-charts' --force-update helm upgrade --install opentelemetry-kube-stack open-telemetry/opentelemetry-kube-stack \\n --namespace opentelemetry-operator-system2 \\n --values './values.yaml' \\n --version '0.3.3'

User needs to:

  1. Update https://github.com/elastic/opentelemetry/blob/8.16/resources/kubernetes/operator/helm/values.yaml#L721
  2. And for instrumentation the url https://github.com/elastic/opentelemetry/blob/8.16/resources/kubernetes/operator/helm/values.yaml#L936
  3. And of course the annotations to have correct namespace later on the manual steps of annotating the deployments' apps

But I agree lets create a new issue to test namespace change. And for now lets remove any reference to namespace configuration. WDYT?

eedugon commented 2 weeks ago

@gizas , thanks a lot for your comments! For all the values.yaml related comments I was going to address that in a different PR (as soon as it was clear where exactly the values file is published), but I think I can manage it on this PR too.

Replying inline:

I am little worried with the compatibility matrix that we can forget to update as we upgrade the versions. Can we make sure that we make this update part of DoD somewhere or automation step?

Good point, let's discuss it in a different issue. I will create one.

The link Values files are available in the resources directory points to a custom reference, I would say to link it with main Also in the matrix can you update the 8.16.0 values.yaml to have a link to https://github.com/elastic/opentelemetry/blob/8.16/resources/kubernetes/operator/helm/values.yaml and the chart version to be 0.3.3 !!!

Perfect, I'll work on that on this PR as it's a small and needed change.

For the namespace I did a smoke test and worked for me

Cool, I'm removing then the NOTE about not being supported and we can add the procedure to change the namespace in a different issue + PR (if we have a way to not hardcode the namespace in the values.yaml that would be ideal :) ).

Thanks a lot for your help!

eedugon commented 2 weeks ago

Changes implemented @gizas , feel free to take a look.