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

Allow configuring more options for output configuration #2089

Merged

Conversation

FrederikNJS
Copy link
Contributor

What this PR does / why we need it:
The TenantID, BatchWait, BatchSize and AutoKubernetesLabels options in the fluent-bit loki output plugin were not configurable. This change allows configuring those options via the helm chart.

Which issue(s) this PR fixes:
None

Special notes for your reviewer:
All the values set in the values file should match the default values found here: https://github.com/grafana/loki/blob/master/cmd/fluent-bit/README.md#configuration-options

Checklist

  • Documentation added
  • Tests updated

The `TenantID`, `BatchWait`, `BatchSize` and `AutoKubernetesLabels` options in the fluent-bit loki output plugin were not configurable. This change allows configuring those options via the helm chart.
@CLAassistant
Copy link

CLAassistant commented May 18, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

One minor nit.
Otherwise LGTM 👏

production/helm/fluent-bit/README.md Outdated Show resolved Hide resolved
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This looks good, just a few comments.

  1. I'm a little worried that setting the default values in helm makes it harder to change them in the future because we'll have to make identical edits in two places. Can we instead inject those configs into the fluent-bit configmap only if defined? On the other hand, these options may be more discoverable with default entries in the helm config... 🤔
  2. I'm hesitant to encourage the AutoKubernetesLabels because iirc it ends up encouraging many small log streams in loki, which is not ideal. We've increasingly been discovering that being scarce with labels has been more performant. This is not a fault of this PR at all -- just something that made me stop and think.
  3. Thanks for the review, I loved to see that!

Looking forward to getting this merged and excited to see some multi-tenant uses.

@cyriltovena @slim-bean WDYT?

Co-authored-by: Joseph Petersen <josephp90@gmail.com>
loglevel: warn
lineFormat: json
k8sLoggingParser: "Off"
removeKeys:
- kubernetes
- stream
autoKubernetesLabels: false
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that this is default false is fine by me.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit 156023a into grafana:master May 26, 2020
mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
* Allow configuring more options for output configuration

The `TenantID`, `BatchWait`, `BatchSize` and `AutoKubernetesLabels` options in the fluent-bit loki output plugin were not configurable. This change allows configuring those options via the helm chart.

* typo in production/helm/fluent-bit/README.md

Co-authored-by: Joseph Petersen <josephp90@gmail.com>

Co-authored-by: Joseph Petersen <josephp90@gmail.com>
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants