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

Helm chart #642

Merged
merged 63 commits into from
Apr 5, 2024
Merged

Helm chart #642

merged 63 commits into from
Apr 5, 2024

Conversation

khushijain21
Copy link
Contributor

Helm Chart for Beyla

khushijain21 and others added 30 commits February 22, 2024 14:08
chart.yaml
adding rbac with changes for labels and annotations

Signed-off-by: Syed Nihal <[email protected]>

adding rbac with changes for labels and annotations

Signed-off-by: Syed Nihal <[email protected]>

adding rbac with changes for labels and annotations

Signed-off-by: Syed Nihal <[email protected]>
Helm 102 - Configmap and env value for external secret
@mariomac
Copy link
Contributor

mariomac commented Mar 26, 2024

Mostly LGTM. @arramos84 do you think is worth merging this PR as a starting point for the consolidation with your chart, or it's better to wait?

@arramos84
Copy link
Contributor

Mostly LGTM. @arramos84 do you think is worth merging this PR as a starting point for the consolidation with your chart, or it's better to wait?

@mariomac, this chart has errors out of the box. At a minimum, it should work with the default values. One error (among others) when deploying is a nil pointer since no default ingress values are defined.

@khushijain21
Copy link
Contributor Author

@arramos84 you are right, we forgot to remove all of ingress related code. Changed now!

Comment on lines 123 to 145
# open_port: 8443
# routes:
# unmatched: heuristic
# log_level: info
# otel_traces_export:
# endpoint: http://grafana-agent:4318
# ## or alternatively use
# # grafana:
# # otlp:
# # cloud_zone: prod-eu-west-0
# # cloud_instance_id: 123456
# # cloud_api_key:
# discovery:
# services:
# - k8s_namespace: default
# attributes:
# kubernetes:
# enable: true
# internal metrics reporting. Refer: https://grafana.com/docs/beyla/latest/configure/options/#internal-metrics-reporter
# If set, user can expose the metrics endpoint via k8s service by configuring .Values.service section
#prometheus_export:
#port: 9090
#path: /metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

@arramos84

At a minimum, it should work with the default values.

That's an interesting point. Beyla requires at least two input values: the service to instrument and the way to export the data (e.g. the OpenTelemetry endpoint or the Prometheus exporter pod).

So I guess that, to let Beyla work with some default inputs, we should provide something like:

# look for ALL the services in the host
discovery:
  services:
    - k8s_namespace: .
# export metrics as prometheus metrics by default
prometheus_export:
  port: 9090
# enable kubernetes
attributes:
  kubernetes:
    enable: true

Could we set the above YAML as the default configMapData?

deployments/helm/values.yaml Outdated Show resolved Hide resolved
deployments/helm/values.yaml Outdated Show resolved Hide resolved
@wasim-nihal
Copy link
Contributor

@mariomac , @petewall , I have addressed the commented in the latest commit.

Changes include:

  • Having default configurations to look for all services.
# look for ALL the services in the host
discovery:
  services:
    - k8s_namespace: .
# export metrics as prometheus metrics by default
prometheus_export:
  port: 9090
# enable kubernetes
attributes:
  kubernetes:
    enable: true
  • change the values.yaml format for config as below
config:
 create: true
 name: "" # Empty means use the release name, otherwise use this to point to an existing configmap
 data:
   <yaml formatted default data>
  • Aligning image tags rendering similar grafana agent chart.
  • Updated Readme
  • Few other corrections and refactoring

@@ -0,0 +1,19 @@
{{- if and (not .Values.config.create) (eq .Values.config.name "") }}
{{- fail "if .Values.config.name is not set, then .Values.config.create should be set to true to use default configuration" }}

Choose a reason for hiding this comment

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

🤩

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! This looks very good now. I think we can merge unless there are any objections and we should be able to build tests with this.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.94%. Comparing base (58a6a7f) to head (6c266ea).
Report is 10 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #642       +/-   ##
===========================================
- Coverage   76.04%   37.94%   -38.10%     
===========================================
  Files         102      102               
  Lines        8165     8172        +7     
===========================================
- Hits         6209     3101     -3108     
- Misses       1614     4890     +3276     
+ Partials      342      181      -161     
Flag Coverage Δ
integration-test ?
unittests 37.94% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mariomac
Copy link
Contributor

mariomac commented Apr 5, 2024

I think that, before merging, some of the contributors still need to agree the license/cla:

image

@khushijain21 @wasim-nihal

Thanks!

@khushijain21
Copy link
Contributor Author

@mariomac yes, just signed it

@mariomac
Copy link
Contributor

mariomac commented Apr 5, 2024

Ok! Then let's merge the PR. If someone sees any issue we can address it in another PR.

@khushijain21 @wasim-nihal thank you a lot for your contribution and your patience!

@mariomac mariomac merged commit 763995b into grafana:main Apr 5, 2024
4 checks passed
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.

9 participants