-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(operator): Add support for Loki OTLP limits config #13446
Conversation
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.
Didn't test, some comments
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.
Still have to test, some other "nits"
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
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.
First round of comments.
I noticed that IgnoreDefaults
makes it easy to create a non-working configuration by specifying no index labels. Do we maybe need a validation for that?
Also this is another instance of the "array with objects where all attributes are optional"... maybe we also need a validation to guard against "all attributes empty"?
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
231c07c
to
9914bdb
Compare
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.
Seems to work nicely in test cluster. 👍
Mostly corrected typos/wording. Only thing is a missing validation.
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.
Sorry for another round. Forgot to add one comment yesterday and also missed one thing in the config generation.
OTLPAttributeActionIndexLabel OTLPAttributeAction = "index_label" | ||
// OTLPAttributeActionStructuredMetadata stores an attribute as structured metadata with each log entry. | ||
OTLPAttributeActionStructuredMetadata OTLPAttributeAction = "structured_metadata" |
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.
Noticed yesterday, but forgot to add the comment... 🙄
Do we want to follow any existing convention with the enum values? Official Kubernetes is CamelCase
, our convention in Loki Operator so far seems to be camelCase
and not snake_case
like in the Loki configuration.
OTLPAttributeActionIndexLabel OTLPAttributeAction = "index_label" | |
// OTLPAttributeActionStructuredMetadata stores an attribute as structured metadata with each log entry. | |
OTLPAttributeActionStructuredMetadata OTLPAttributeAction = "structured_metadata" | |
OTLPAttributeActionIndexLabel OTLPAttributeAction = "indexLabel" | |
// OTLPAttributeActionStructuredMetadata stores an attribute as structured metadata with each log entry. | |
OTLPAttributeActionStructuredMetadata OTLPAttributeAction = "structuredMetadata" |
(needs to also update the validations if changed)
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
operator/internal/manifests/internal/config/loki-runtime-config.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Jacob <[email protected]>
What this PR does / why we need it:
The following PR adds support to the Loki Operator to handle OTLP configuration (global+per-tenant) on how to handle indexing fields from log streams ingested through Loki 3.x OTLP endpoint. The configuration options are provided in the LokiStack CRD below the
LimitsSpec
. Example LokiStack with this change:Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
It's worthwhile to consider the following Loki docs pages:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR