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

xperimental · 2024-10-07T18:20:02Z

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

… 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

periklis

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

Question: How do we define the drop action?

operator/internal/manifests/config_otlp.go

periklis · 2024-10-15T11:53:58Z

operator/internal/manifests/config_otlp.go

+			ResourceAttributes: []config.OTLPAttribute{
+				{
+					Action: config.OTLPAttributeActionStreamLabel,
+					Regex:  "openshift\\.labels\\..+",


Question: What is considered an OpenShift Label here?

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.

periklis · 2024-10-15T11:56:02Z

operator/internal/manifests/config_otlp.go

+	"github.com/grafana/loki/operator/internal/manifests/internal/config"
+)
+
+func defaultOpenShiftLoggingAttributes(disableRecommended bool) config.OTLPAttributeConfig {


Can we move this function to the openshift package please? This entire file mingles openshift-related with generic otlp handling. We use to have configure* function pattern to patch with mergo when an openshift mode is selected, else pass it through.

I think mergo would overcomplicate things here, but I can move the OpenShift defaults to a separate package if we want that distinction.

periklis · 2024-10-15T13:10:43Z

operator/internal/manifests/config_otlp.go

+						"k8s.cronjob.name",
+						"k8s.daemonset.name",
+						"k8s.deployment.name",
+						"k8s.job.name",
+						"k8s.replicaset.name",
+						"k8s.statefulset.name",
+						"process.command_line",
+						"process.executable.name",
+						"process.executable.path",
+						"process.pid",
+						"service.name",


Suggest to move the following to stream labels:

"k8s.cronjob.name", "k8s.daemonset.name", "k8s.deployment.name", "k8s.statefulset.name", "service.name",

I don't think we have data for any of these attributes yet, but it might make sense in the long run... (this also needs an update to data-model if we do this)

JoaoBraveCoding

First pass without testing or looking at the test cases.

operator/apis/loki/v1/lokistack_types.go

JoaoBraveCoding · 2024-10-15T11:18:31Z

operator/apis/loki/v1/lokistack_types.go

+	//
+	// This option is supposed to be combined with a custom label configuration customizing the labels for the specific
+	// usecase.
+	//


Should we mention that some attributes will still be configured even if this is enabled?

operator/apis/loki/v1/lokistack_types.go

JoaoBraveCoding · 2024-10-15T11:24:13Z

operator/apis/loki/v1/lokistack_types.go

-	// altogether.
+// TenantOTLPSpec defines an OTLP attribute configuration specific to a tenant. It extends OTLPSpec.
+type TenantOTLPSpec struct {
+	// When IgnoreGlobalStreamLabels is true, then this tenant will ignore the global stream label configuration.


Suggested change

// When IgnoreGlobalStreamLabels is true, then this tenant will ignore the global stream label configuration.

// When IgnoreGlobalStreamLabels is true, then this tenant will ignore the global stream labels configuration.

nit: should we clarify here what happens if you have global stream labels defined and the users also defines them for a tenant?

I don't think the plural ("labels") is correct, because "configuration" already implies a plural and you don't "pluralize a plural"? I might be confusing this with German rules (or personal preference), though.

Regarding the other point: Valid point, this might need more thought... For example if we need to account for this in the deduplication as well.

operator/internal/manifests/config_otlp.go

operator/internal/manifests/internal/config/loki-config.yaml

xperimental

Added the simple changes, but the comments about the "ignore defaults" made me check how this is actually used in Loki and it doesn't match my expectation. I think we need to decide whether we want a "ignore global attributes" setting and, if yes, implement it ourselves. Let's quickly discuss this in the daily. Implementation then probably on Monday.

xperimental · 2024-10-18T10:29:09Z

operator/apis/loki/v1/lokistack_types.go

-	// altogether.
+// TenantOTLPSpec defines an OTLP attribute configuration specific to a tenant. It extends OTLPSpec.
+type TenantOTLPSpec struct {
+	// When IgnoreGlobalStreamLabels is true, then this tenant will ignore the global stream label configuration.


I don't think the plural ("labels") is correct, because "configuration" already implies a plural and you don't "pluralize a plural"? I might be confusing this with German rules (or personal preference), though.

Regarding the other point: Valid point, this might need more thought... For example if we need to account for this in the deduplication as well.

xperimental · 2024-10-18T10:31:11Z

operator/internal/manifests/config_otlp.go

+	"github.com/grafana/loki/operator/internal/manifests/internal/config"
+)
+
+func defaultOpenShiftLoggingAttributes(disableRecommended bool) config.OTLPAttributeConfig {


I think mergo would overcomplicate things here, but I can move the OpenShift defaults to a separate package if we want that distinction.

xperimental · 2024-10-18T10:33:15Z

operator/internal/manifests/config_otlp.go

+			ResourceAttributes: []config.OTLPAttribute{
+				{
+					Action: config.OTLPAttributeActionStreamLabel,
+					Regex:  "openshift\\.labels\\..+",


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.

xperimental · 2024-10-18T10:35:32Z

operator/internal/manifests/config_otlp.go

+						"k8s.cronjob.name",
+						"k8s.daemonset.name",
+						"k8s.deployment.name",
+						"k8s.job.name",
+						"k8s.replicaset.name",
+						"k8s.statefulset.name",
+						"process.command_line",
+						"process.executable.name",
+						"process.executable.path",
+						"process.pid",
+						"service.name",


I don't think we have data for any of these attributes yet, but it might make sense in the long run... (this also needs an update to data-model if we do this)

operator/internal/manifests/config_otlp.go

operator/internal/manifests/internal/config/loki-config.yaml

xperimental · 2024-10-18T13:46:23Z

Updated the PR to reflect some of the changes we discussed during the daily. The CRD already reflects the final view, but the "copy global to tenant configuration" part is still missing. The current version still provides defaults useful for testing with CLO, though.

xperimental added 5 commits October 7, 2024 19:39

Extend API for regular expressions

67c88b2

Make conversion code compatible with regex

bb420c9

Fix typo

9da80cd

Update default attributes based on data model

b2e70aa

xperimental self-assigned this Oct 7, 2024

pull-request-size bot added the size/XXL label Oct 7, 2024

github-actions bot added the sig/operator label Oct 7, 2024

xperimental added 7 commits October 7, 2024 20:29

Split OTLP config to separate file

9919c58

Add tests for OTLP config conversion

8258bbd

Update API docs

7094618

Merge branch 'main' into otlp-defaults

5d13142

Update test after Go update

b2209f2

Small refactor of default config

09daa17

Convert per-tenant attributes in configuration

3a43315

xperimental force-pushed the otlp-defaults branch from 3811195 to 3a43315 Compare October 11, 2024 14:57

xperimental added 8 commits October 11, 2024 17:15

Refactor conversion code

1157d1a

Split default attributes into required and recommended

8ef3697

Merge branch 'main' into otlp-defaults

35ff750

Test for default mixed with custom attributes

b5986b6

Simple attribute de-duplication

8ab9a3f

Sort and deduplicate attributes

03a2236

Provide defaults pre-sorted

1071aa8

Add kubernetes_host stream label

207d3bd

xperimental force-pushed the otlp-defaults branch from 80c45e9 to 207d3bd Compare October 14, 2024 20:45

xperimental marked this pull request as ready for review October 15, 2024 10:56

xperimental requested review from periklis, JoaoBraveCoding and a team as code owners October 15, 2024 10:56

periklis reviewed Oct 15, 2024

View reviewed changes

JoaoBraveCoding reviewed Oct 15, 2024

View reviewed changes

xperimental added 2 commits October 18, 2024 12:48

Simple changes from review

47c7b93

Merge branch 'main' into otlp-defaults

5d6955f

xperimental commented Oct 18, 2024

View reviewed changes

xperimental added 3 commits October 18, 2024 13:15

Update bundle and docs

40840bf

Remove duplicate change introduced by merge

1f5d5b9

Remove option to ignore global configuration

1e3dac8

xperimental force-pushed the otlp-defaults branch from c1547b0 to 1e3dac8 Compare October 18, 2024 13:26

xperimental added 3 commits October 18, 2024 15:28

Small refactor of defaults function

630f04d

Update todo lines

0dccac4

Update API docs

88bf863

xperimental added the DO NOT MERGE label Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xperimental commented Oct 7, 2024 •

edited

Loading

periklis left a comment

periklis Oct 15, 2024

xperimental Oct 18, 2024

periklis Oct 15, 2024

xperimental Oct 18, 2024

periklis Oct 15, 2024

xperimental Oct 18, 2024

JoaoBraveCoding left a comment

JoaoBraveCoding Oct 15, 2024

JoaoBraveCoding Oct 15, 2024

JoaoBraveCoding Oct 15, 2024

xperimental Oct 18, 2024

xperimental left a comment

xperimental Oct 18, 2024

xperimental Oct 18, 2024

xperimental Oct 18, 2024

xperimental Oct 18, 2024

xperimental commented Oct 18, 2024

	// When IgnoreGlobalStreamLabels is true, then this tenant will ignore the global stream label configuration.
	// When IgnoreGlobalStreamLabels is true, then this tenant will ignore the global stream labels configuration.

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

Are you sure you want to change the base?

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

Conversation

xperimental commented Oct 7, 2024 • edited Loading

periklis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xperimental left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xperimental commented Oct 18, 2024

xperimental commented Oct 7, 2024 •

edited

Loading