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

feat(operator)!: Add configuration option for dropping OTLP attributes #15857

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

xperimental
Copy link
Collaborator

@xperimental xperimental commented Jan 21, 2025

What this PR does / why we need it:

The Loki Operator already supports configuring whether to store OTLP attributes as stream labels or structured metadata, but the option to drop attributes was omitted from the LokiStack API, because of a misunderstanding that "drop" is the default behavior (the actual default behavior is "structured metadata".

This PR adds the drop capability to the LokiStack API as well.

Which issue(s) this PR fixes:

Fixes LOG-6507.

Special notes for your reviewer:

  • If "structured metadata" is the default behavior, I wonder if we should remove the StructuredMetadata attribute of the LokiStack (instead of just deprecating it). I think it's still a source of confusion for the user and removing it would also reduce the code needed in the operator quite a bit (mostly validation). This would be a breaking change though.
    • We have decided to include the breaking change. I'll do a bit more testing tomorrow and will take the PR out of draft then.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added (Loki Operator website will be updated in a separate PR)
  • Tests updated
  • Title matches the required conventional commits format, see here

@xperimental xperimental changed the title feat(operator): Add configuration option for dropping OTLP attributes feat!(operator): Add configuration option for dropping OTLP attributes Jan 22, 2025
Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding left a 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 but overall lgtm. Good job 🙌

Small nit comment regarding the use of errList = append(errList, ...). Sometimes we use this with just an error where other times with a function call. Although this makes the code more compact I feel that sometimes it makes it a bit harder to follow. Maybe moving instances with function calls to an if and only appending to errList if the result yields a list of errors. This is just a suggestion feel free to ignore if you feel it will not improve the code maintenance.

operator/api/loki/v1/v1.go Outdated Show resolved Hide resolved
operator/internal/validation/openshift/otlp.go Outdated Show resolved Hide resolved
operator/internal/validation/openshift/otlp.go Outdated Show resolved Hide resolved
@xperimental xperimental changed the title feat!(operator): Add configuration option for dropping OTLP attributes feat(operator)!: Add configuration option for dropping OTLP attributes Jan 23, 2025
@xperimental xperimental marked this pull request as ready for review January 23, 2025 16:42
@xperimental xperimental requested review from periklis and a team as code owners January 23, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants