-
Notifications
You must be signed in to change notification settings - Fork 459
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 otel-cloud-stack chart #1058
Conversation
okay. so now this thing actually does something!! Next week I have to test out the kube-otel-stack functionality. But you can try for yourself by layering this values file over the existing values. (obviously this works for LS and youll needd to change to your vendor of choice ;)) Here's that config
|
@@ -0,0 +1,3 @@ | |||
apiVersion: v2 |
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 matches what's done in kube-prometheus-stack such that we can install the necessary prometheus CRDs in the right order. the installation of this is entirely optional and disabled by default.
@jaronoff97 it would be nice to have some self-monitoring of the operator and collectors? Not sure what form that would take (e.g. ServiceMonitor or static scrape job). |
this is actually already possible and you can take a look at that in my sample configs in the |
@jaronoff97 I see you can bring your own ServiceAccount for your collector and target allocator. However, the ClusterRoleBinding is using the ServiceAccount that the operator is generating, not your override. |
ah great catch! thank you. |
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.
@jaronoff97 thanks for following through with this, I am very excited about it.
This is a large PR adding a lot of features. Is there anything we can do to make it smaller? My initial thought is we could start with it only installing the operator/collector and then in followup PRs introduce the target allocator and all the prom stuff.
Please add:
- a README for the chart
- a ci folder with an empty values.yaml so we can test the default install
- please update the github workflow CI to test the chart.
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'm guessing you used this to generate the schema file, do we need to check it in?
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.
nope, i was thinking it may be helpful for future schema generation! but am happy to leave it out :)
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 agree that it could be helpful in the future for all the charts. Let's add that in a separate pr.
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.
sounds good!
@@ -0,0 +1,29 @@ | |||
apiVersion: v2 | |||
name: otel-cloud-stack | |||
version: 0.1.0 |
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'd rather start with 0.0.1
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.
sounds good!
- name: jaronoff97 | ||
- name: anammedina21 |
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.
Please list @TylerHelmuth @dmitryax and @Allex1. I'd love if you wanted to help maintain this chart and become an approver after it is merged, we did something similar with @puckpuck and the demo chart.
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 do!
version: "0.0.0" | ||
condition: prometheus.customResources.enabled | ||
- name: opentelemetry-operator | ||
repository: "file://../opentelemetry-operator" |
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.
Let's use https://open-telemetry.github.io/opentelemetry-helm-charts
, thats what we do for the demo.
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 do!
- name: jaronoff97 | ||
- name: anammedina21 | ||
icon: https://raw.githubusercontent.com/cncf/artwork/a718fa97fffec1b9fd14147682e9e3ac0c8817cb/projects/opentelemetry/icon/color/opentelemetry-icon-color.png | ||
appVersion: 0.94.0 |
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 appversion corresponds with the collector, but the sources only list the operator.
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.
yeah i was thinking this would determine the version for everything this chart creates (read: collectors) but i also should pipe this in to the image used for the operator too
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.
What does this files do? It isn't part of template
so it isn't included in any install of the chart. Is it an expected scrape config to use with the chart?
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 actually included in the chart via the logic in the collector.yaml. The chart dynamically pulls these values in and appends them (or creates a new) prometheus scrape config in the prometheus receiver
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.
Should we move them in a configs
folder?
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.
What does this files do? It isn't part of template
so it isn't included in any install of the chart. Is it an expected scrape config to use with the chart?
# Top level field related to the OpenTelemetry Operator | ||
opentelemetry-operator: | ||
# Field indicating whether the operator is enabled or not | ||
enabled: false |
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.
Should this be enabled by default? The goal of the chart is to install an operator/collector all in one go right?
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.
yep probably! for my testing it was easier to not have it by default initially, but i can change that.
|
||
# This is the default configuration for all collectors generated by the chart. | ||
# Any collectors in the `collectors` are overlayed on top of this configuration. | ||
defaultCollectorConfig: |
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 this name is misleading - it implies to me it configures the collector's configuration when it is configuring the OpenTelemetryCollector resource. Could we name it opentelemetry-collector
?
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.
see comment below for why this is done, i agree this could probably have a better name. baseCollectorCRConfig
?
# mountPath: /etc/config | ||
|
||
# Top level field specifying collectors configuration | ||
collectors: |
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'd prefer to nest all things related to the collector under 1 root section.
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.
the benefit of doing it this way is that anyone can add any collectors they want created to this key and they will be created as part of the chart. It also allows them to easily overlay values for specific collectors in different environments. i.e. if someone wanted to make a custom collector xcol
and have a different sample rate in each env/region combo ([dev,prod] x [us-east-1, us-east-2]) they would be able to have a single xcol definition and only overlay the new sample rate rather than redefine the entire configuration.
@TylerHelmuth i think i could split this up in to the following sections:
do i need to also make a rendered folder to test that? |
@jaronoff97 I like that approach.
We can handle this via an example folder like the other charts. |
@TylerHelmuth I see, so we would have some files like example/kube-prom-stack which has the values files overrides for doing that? This, rather than baking in these defaults? |
I am not sure yet, I need to understand the how the default config files are used. The examples folder is strictly for viewing, they shouldn't be used or referenced in any templates |
i see, in this case the chart is meant to serve as a quickstart with opentelemetry. Hopefully giving users full visibility in to their kube clusters the "otel" way. Or allowing people on prometheus with the kube-prometheus-stack a near exact replica but with the collector |
@jaronoff97 yup, I was referring to those yaml files. I may not understand what you meant by rendered folder. Regardless, let's move forward with incremental PRs. |
Closing this for now in favor of the approach specified here |
Why make this?
This PR serves to finally close #562. This PR creates a helm chart that takes some inspiration from the kube-prometheus-stack chart from the prometheus community repo to create a fully-fledged collector starter for Kubernetes. Prometheus services and CRDs are optionally included to allow users a near-drop-in replacement for the prometheus experience. For non-prometheus users, this provides an easy way to get started with opentelemetry's Kubernetes offerings. This is accomplished by utilizing some clever templating for the configuration for the collector, which I found to be easier to reason about than embedding config in a helper tpl.
Features
Next steps
For a config that gets you tracing, infra logs and metrics, kube cluster stats
For a config that's a near drop in for kube-prometheus-stack