Skip to content

feat(operator)!: Provide default OTLP attribute configuration - #14410

Merged
xperimental merged 43 commits into
grafana:mainfrom
xperimental:otlp-defaults
Oct 23, 2024
Merged

feat(operator)!: Provide default OTLP attribute configuration#14410
xperimental merged 43 commits into
grafana:mainfrom
xperimental:otlp-defaults

Conversation

@xperimental

@xperimental xperimental commented Oct 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:

The main purpose of this PR is to provide a default set of OTLP attributes that will be persisted when using LokiStack in openshift-logging tenancy mode.

This PR also changes the API for specifying an OTLP attribute configuration in LokiStack to make it easier to use.

Which issue(s) this PR fixes:

LOG-6147

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
@xperimental
wip! feat(operator): Provide default OTLP attribute configuration for…
3c253af 
… openshift-logging tenancy

- Extend API for default OTLP label set
- Disable autodetection of service.name
- Update non-otlp config tests
- Simplify OTLP attribute configuration
- Remove validator code
- Content-assist for runtime config
- Add openshift-logging defaults
- Only add ignore_defaults to config if needed
- Use Loki default OTLP attributes when none are specified
@xperimental
Extend API for regular expressions
67c88b2
@xperimental
Make conversion code compatible with regex
bb420c9
@xperimental
Fix typo
9da80cd
@xperimental
Update default attributes based on data model
b2e70aa
@xperimental xperimental self-assigned this Oct 7, 2024
@xperimental
xperimental marked this pull request as ready for review October 15, 2024 10:56
periklis
periklis reviewed Oct 15, 2024
View reviewed changes

@periklis periklis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

quick first pass, testing this right now on my cluster.

Question: How do we define the drop action?
Comment thread operator/internal/manifests/config_otlp.go Outdated
Comment thread operator/internal/manifests/config_otlp.go Outdated
ResourceAttributes: []config.OTLPAttribute{
{
Action: config.OTLPAttributeActionStreamLabel,
Regex: "openshift\\.labels\\..+",

@periklis periklis Oct 15, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Question: What is considered an OpenShift Label here?

@xperimental xperimental Oct 18, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there are any of these labels at the moment. This might be an extension point. I meant to write a question about this in the data-model PR, but I forgot. The attribute configuration itself is from the data-model repository.
Comment thread operator/internal/manifests/config_otlp.go Outdated
@xperimental

xperimental commented Oct 21, 2024

Copy link
Copy Markdown
Collaborator Author

Updated with the changes discussed on Friday. ✔️
periklis
periklis reviewed Oct 22, 2024
View reviewed changes

@periklis periklis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall LGTM just one question to clarify the non-openshift mode handling. Besides that I suggest to create a follow up PR for a documentation page explaining the append as well as the per-tenant full-override workings of the OTLPSpec. WDYT?
Comment thread operator/internal/manifests/config_otlp.go
JoaoBraveCoding
JoaoBraveCoding reviewed Oct 22, 2024
View reviewed changes

@JoaoBraveCoding JoaoBraveCoding left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apart from what was discussed in the daily lgtm 👍 still didn't manually tested it

image
@xperimental xperimental changed the title feat(operator): Provide default OTLP attribute configuration Oct 22, 2024
periklis
periklis approved these changes Oct 23, 2024
View reviewed changes

@periklis periklis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🎉
JoaoBraveCoding
JoaoBraveCoding approved these changes Oct 23, 2024
View reviewed changes

@JoaoBraveCoding JoaoBraveCoding left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm 💪
@xperimental
xperimental merged commit 1b52387 into grafana:main Oct 23, 2024
@xperimental
xperimental deleted the otlp-defaults branch October 23, 2024 10:59
@loki-gh-app loki-gh-app Bot mentioned this pull request Oct 23, 2024
Merged
@xperimental xperimental mentioned this pull request Oct 23, 2024
Merged
This was referenced Nov 4, 2024
Merged
Merged
@loki-gh-app loki-gh-app Bot mentioned this pull request Nov 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/operator size/XXL

3 participants

@xperimental @periklis @JoaoBraveCoding