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

[nr-k8s-otel-collector] Honor the LowDataMode toogle from the common-library #1472

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kang-makes
Copy link
Contributor

Is this a new chart

No

What this PR does / why we need it:

The value for lowDataMode was hard coded at values level so a user cannot set the value if you not go deep down to the chart and patch it.

This change has its own implementation of the lowDataMode helper that defaults to true while honoring the behaviour of the common-library of setting a value a global level.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [mychartname])

@kang-makes kang-makes requested a review from a team as a code owner September 3, 2024 14:32
@@ -153,5 +153,5 @@ receivers:
enabled: true

# -- (bool) Send only the [metrics required](https://github.com/newrelic/helm-charts/tree/master/charts/nr-k8s-otel-collector/docs/metrics-lowDataMode.md) to light up the NR kubernetes UI, this agent defaults to setting lowDataMode true, but if this setting is unset, lowDataMode will be set to false
Copy link
Contributor

Choose a reason for hiding this comment

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

After this change, wouldn't lowDataMode be true if unset? If so, we should update this wording.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kang-makes Bumping this :) Thanks!

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.

3 participants