Add option to disable redundant 132B in log lines

[yaten2302]

Description

This PR adds an option to disable the resource attribute in logs.

Link to tracking issue

Fixes #13869

Testing

Added unit tests in - TestCreateLogger().

[yaten2302]

There're some failing tests, working on the fix. By the time, I've converted this to draft 👍

[yaten2302]

@jade-guiton-dd , wanted to ask here, that currently, TestCreateLoggerWithResource test is failing in logger_test.go.
If I explicitly mention like this:

{
			name: "auto-populated fields only",
			buildInfo: component.BuildInfo{
				Command: "mycommand",
				Version: "1.0.0",
			},
			resourceConfig: map[string]*string{},
			wantFields: map[string]string{
				string(semconv.ServiceNameKey):       "mycommand",
				string(semconv.ServiceVersionKey):    "1.0.0",
				string(semconv.ServiceInstanceIDKey): "",
			},
			cfg: &Config{
				Logs: LogsConfig{
					Level:               zapcore.InfoLevel,
					Encoding:            "json",
					ResourceAsZapFields: true,
				},
				Resource: map[string]*string{},
			},
		},

Then, it'll work fine, but, it's not working if I don't add this field - ResourceAsZapFields. How, can I make it's value as true by default?
I tried using the defaults lib as well - "github.com/mcuadros/go-defaults". But, still the default value isn't being set as true. I'm trying to figure this out, if you've any suggestions/hints for this, kindly share :)

[jade-guiton-dd]

The default configuration for the telemetry factory is in service/telemetry/otelconftelemetry/factory.go, you'll want to modify the createDefaultConfig function to set the new field.

[yaten2302]

@jade-guiton-dd , I've added the required field in the createDefaultConfig() func in factory.go. But, if we check in the TestCreateLoggerWithResource() func, we're passing our own config (currently that code is commented for testing purpose). So, if we pass our own config, then the default one will be overwritten, right? Because, if I test this func by passing - createDefaultConfig to createLogger func, then the 2 tests are passing and others which are failing are because it's meant to be (because of wantFields).

In short, the as far as I've observed here is that, this - if cfg.Logs.ResourceAsZapFields && res != nil && len(res.Attributes()) > 0 { (which I added) is working fine, but, I'm in a dilemma that if we pass a custom config, then what should be the behaviour? Should it merge the default and the custom config or the default config shouldn't be taken into account (while passing the default one)? Since, the current behaviour in my latest commit is this only. The default config is getting ignored if we pass the custom one, and if we pass the default config (in which we've ResourceAsZapFields as true by default), it's working perfectly fine.

[jade-guiton-dd]

I didn't fully understand your comment, but let me try to explain.

The way defaults are normally handled in the Collector is as follows: 1. the createDefaultConfig function is called, which returns a config struct with default values, 2. the user's YAML config is parsed and merged into this struct, overwriting fields explicitly set by the user and leaving others as-is, 3. the resulting struct is passed to the component.

In tests however, we need to perform this logic manually. If you pass in a config struct to a component, and a field is not specified, that field will default to its zero value, as is normal in Go; the value of that field in createDefaultConfig doesn't matter.

This means that the existing test cases in TestCreateLogger already have ResourceAsZapFields set to false. In that regard, I think it would make more sense for the new test case you added to set ResourceAsZapFields: true instead, to ensure that it doesn't cause issues for logger creation.

As for TestCreateLoggerWithResource: since that test is specifically checking the behavior that ResourceAsZapFields is toggling, it makes sense to hardcode ResourceAsZapFields: true in the config that it uses.

[yaten2302]

Got this, thanks for explaining @jade-guiton-dd :)
I've pushed a commit for this, and the test TestCreateLoggerWithResource is passing now

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.13%. Comparing base (1197fba) to head (3a1e666).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14050      +/-   ##
==========================================
- Coverage   92.16%   92.13%   -0.03%     
==========================================
  Files         668      668              
  Lines       41516    41517       +1     
==========================================
- Hits        38263    38252      -11     
- Misses       2217     2225       +8     
- Partials     1036     1040       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jade-guiton-dd]

Yes, I think that's the best option.

[jade-guiton-dd]

Looks pretty good to me. Before I approve though: @bogdandrutu, can you confirm whether this is what you had in mind in #13869?

[yaten2302]

I'll address the suggested changes by the end of week👍 Got busy with some work.

[codspeed-hq]

CodSpeed Performance Report

Merging #14050 will degrade performances by 77.03%

_{Comparing yaten2302:main (90fc3cd) with main (a0cbea7)}

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

❌ 2 regressions
✅ 71 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	zstdNoConcurrency	36.3 µs	74.8 µs	-51.51%
❌	zstdWithConcurrency	6.9 µs	30 µs	-77.03%

[jade-guiton-dd]

Looks good. Suggested a few documentation changes to make it extra clear what this applies to.

[yaten2302]

@jade-guiton-dd , there're some tests failing after I pulled some changes in this PR, can we still merge this?

[jade-guiton-dd]

No worries, these failures are unrelated to your PR and have since been fixed. I've re-run the tests.