Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @Abhinav1299, and @angles-n-daemons)

-- commits line 7 at r2:
Add specific details about the changes in this commit. I.e It only adds the config parsing logic. the actual implementation will follow in a future commit.

pkg/util/log/logconfig/config.go line 606 at r2 (raw file):

//
// User-facing documentation follows.
// TITLE: Output to OpenTelemetry collectors.

Suggestion:

Output to OpenTelemetry compatible collectors

pkg/util/log/logconfig/config.go line 634 at r2 (raw file):

//	       # unless overridden here.
//
// The default output format for HTTP sinks is

Suggestion:

OTLP

pkg/util/log/logconfig/config.go line 640 at r2 (raw file):

// Run `cockroach debug check-log-config` to verify the effect of defaults inheritance.
// {{site.data.alerts.end}}
type OtlpSinkConfig struct {

The user will eventually be able to pick between GRPc and HTTP right?
I think we should have an option to configure that. We can keep "grpc" as default for now (since we don't have an HTTP implementation)

pkg/util/log/logconfig/validate.go line 523 at r2 (raw file):

func (c *Config) validateOtlpSinkConfig(otsc *OtlpSinkConfig) error {
	propagateOtlpDefaults(&otsc.OtlpDefaults, c.OtlpDefaults)
	otsc.Address = strings.TrimSpace(otsc.Address)

We should not mutate a value in a "validate" function. For trimspace, can we find a more appropriate place? We might not have to do this at all actually.

If this is only used for len(otsc.Address) == 0 check, you can use TimeSpace inside the condition without actually mutating the value also

if len(strings.TimeSpace(otsc.Address)) == 0 {

Also, if the user has not specified the Address, we could consider defaulting to localhost:standard_port. What do you think?

pkg/util/log/flags.go line 450 at r2 (raw file):

func newOtlpSinkInfo(_ logconfig.OtlpSinkConfig) (*sinkInfo, error) {
	return nil, nil

Why is this nil?
Is this left for a separate PR?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @Abhinav1299, @angles-n-daemons, and @arjunmahishi)

-- commits line 7 at r2:

Done.

pkg/util/log/flags.go line 450 at r2 (raw file):

Yes, the actual sink functionality is left for a separate PR.

pkg/util/log/logconfig/config.go line 640 at r2 (raw file):

I'll add the mode option, which will allow to toggle between gRPC and HTTP, in the same PR in which the HTTP mode will be implemented.

pkg/util/log/logconfig/validate.go line 523 at r2 (raw file):

The fluent sink does the same, I don't think its much of an issue as it just removes space from the ends of the string.

I don't think defaulting is a good idea in this case because if we choose to add HTTP support along with gRPC, the default address will change.

pkg/util/log/logconfig/config.go line 606 at r2 (raw file):

//
// User-facing documentation follows.
// TITLE: Output to OpenTelemetry collectors.

Done.

pkg/util/log/logconfig/config.go line 634 at r2 (raw file):

//	       # unless overridden here.
//
// The default output format for HTTP sinks is

Done.

Reviewed 1 of 5 files at r1, 3 of 5 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Abhinav1299, @angles-n-daemons, and @arjunmahishi)

pkg/util/log/flags.go line 450 at r2 (raw file):

Can you add a TODO?

pkg/util/log/logconfig/config.go line 40 at r4 (raw file):

// DefaultOtlpFormat is the entry format for OpenTelemetry sinks
// when not specified in a configuration.
const DefaultOtlpFormat = `json`

qq: Are we planning to support any other format than json?

docs/generated/logsinks.md line 233 at r4 (raw file):

Configuration options shared across all sink types:

Is it auto generated?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @Abhinav1299, @angles-n-daemons, and @arjunmahishi)

docs/generated/logsinks.md line 233 at r4 (raw file):

Yes

pkg/util/log/flags.go line 450 at r2 (raw file):

Done.

pkg/util/log/logconfig/config.go line 40 at r4 (raw file):

Format is sink independent, we can send data in any format as long as the collector is configured to parse it.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aa-joshi, @Abhinav1299, and @angles-n-daemons)

-- commits line 10 at r5:
NIT

Just suggesting a few semantic changes for more clarity

Suggestion:

  This commit only defines basic configuration options for the OTLP sink, like
  address, insecure, and compression, and adds logic to parse them from the YAML
  config. The actual sink implementation will follow in a future commit.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aa-joshi, @Abhinav1299, @angles-n-daemons, and @arjunmahishi)

-- commits line 10 at r5:

Done.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @Abhinav1299, @angles-n-daemons, @arjunmahishi, and @TheComputerM)

pkg/util/log/logconfig/config.go line 40 at r4 (raw file):

👍