Add typed collector resource attributes based on declarative config

[iblancasa]

Description

Implement OpenTelemetry configuration schema for OpenTelemetry Collector resource attributes.

It keeps backward compatibility.

Link to tracking issue

Updates #14411

[codecov]

Codecov Report

❌ Patch coverage is 94.87179% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.32%. Comparing base (f9d2c94) to head (5642768).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
service/telemetry/otelconftelemetry/resource.go	90.90%	3 Missing and 3 partials ⚠️

❌ Your patch status has failed because the patch coverage (94.87%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14412   +/-   ##
=======================================
  Coverage   91.31%   91.32%           
=======================================
  Files         702      700    -2     
  Lines       45265    45314   +49     
=======================================
+ Hits        41334    41381   +47     
- Misses       2781     2782    +1     
- Partials     1150     1151    +1     

☔ 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.

[codspeed-hq]

Merging this PR will improve performance by 41.21%

⚠️ 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.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
✅ 4 untouched benchmarks
⏩ 76 skipped benchmarks¹

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	BenchmarkTraceSizeSpanCount	60 ns	47 ns	+27.66%
⚡	BenchmarkHTTPProtoLogsSequential	5.1 ms	3.6 ms	+40.76%
⚡	BenchmarkGRPCLogsSequential	6.2 ms	4.4 ms	+41.21%

---

_{Comparing iblancasa:14411 (5642768) with main (f9d2c94)}

Footnotes

1. 76 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[jade-guiton-dd]

Do you think it would be possible to use configoptional.MapList instead of reimplementing the fallback logic? I'm not sure if it would work for your use case, but just in case you hadn't heard of it.

[iblancasa]

Do you think it would be possible to use configoptional.MapList instead of reimplementing the fallback logic? I'm not sure if it would work for your use case, but just in case you hadn't heard of it.

Thanks for the suggestion. You comment actually points to configopaque.MapList.

I looked into it.configopaque.MapList only supports configopaque.String values and doesn’t allow nil. I find this can break backward compatibility. Maybe we can implement something like a configopaque.MapList to solve similar situations like this one?

[jade-guiton-dd]

Sorry for the delayed review. I think implementing the declarative config's resource struct in service/internal/resource and using it in service/telemetry/otelconftelemetry is not a good idea and goes against the existing architecture.

Like for the rest of the internal telemetry config, I think we should be relying on otelconf's implementation (see Resource struct here) to implement the config specification, with "migration" logic to preserve backward compatibility with the old format, then use its output directly when instantiating the SDK, and convert it into a pcommon.Resource when necessary.

[iblancasa]

Sorry for the delayed review. I think implementing the declarative config's resource struct in service/internal/resource and using it in service/telemetry/otelconftelemetry is not a good idea and goes against the existing architecture.

Like for the rest of the internal telemetry config, I think we should be relying on otelconf's implementation (see Resource struct here) to implement the config specification, with "migration" logic to preserve backward compatibility with the old format, then use its output directly when instantiating the SDK, and convert it into a pcommon.Resource when necessary.

I see. I will take a look and come back. Thanks!

[jade-guiton-dd]

Thank you for the update. I think there's a lot to simplify, remove, or delegate to otelconf here; we probably don't need +2700 new lines of code for this.

[iblancasa]

Thank you for the update. I think there's a lot to simplify, remove, or delegate to otelconf here; we probably don't need +2700 new lines of code for this.

Let me check. I reused some stuff from the previous changes and other PRs opened before. Very likely there are things in otelconf I'm not aware of. Will take a look to see what I can do.

Thanks again for your fast review.

[jade-guiton-dd]

Thank you for the update. I still think there's some simplification we can do here. I left some suggestions, but haven't tested them, so take them with a grain of salt.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jade-guiton-dd]

Thank you for filing these go-contrib and spec issues; since I'm the one who suggested that change in otelconf, feel free to tag me in those threads. I'll try to chime in if necessary.

[jade-guiton-dd]

Looks good! Thank you for all your work

[dmitryax]

So unless you apply the discouraged practice of having the Collector process its own internal telemetry, it won't work.

Do we mark it as discouraged somewhere?

Additionally, the resource detection processor can't help with the resource attributes exposed via OpAMP, which I'm personally very interested in since the current attributes are a bit lacking.

Will the sdk detectors work with opamp extension/supervisor?

I'm not against this. I'm a bit concerned about two separate solutions in the same binary that could potentially diverge and produce different results. We probably should start using the sdk detectors libraries in the detection processor going forward

[iblancasa]

Will the sdk detectors work with opamp extension/supervisor?

Since we mostly agree on this PR, I will create another one for the supervisor open-telemetry/opentelemetry-collector-contrib#45116

We probably should start using the sdk detectors libraries in the detection processor going forward

Very likely.

[jade-guiton-dd]

So unless you apply the discouraged practice of having the Collector process its own internal telemetry, it won't work.

Do we mark it as discouraged somewhere?

No. We used to document it as a discouraged option on the "Internal telemetry" docs page, but in the end after discussion on the opentelemetry.io repo, it was decided this option was so unreliable it shouldn't even be mentioned. Considering some vendors (*coughs*) recommend it in their own docs regardless, maybe we should reintroduce an explicit warning about it?

[jade-guiton-dd]

Additionally, the resource detection processor can't help with the resource attributes exposed via OpAMP, which I'm personally very interested in since the current attributes are a bit lacking.

Will the sdk detectors work with opamp extension/supervisor?

Yes. The SDK resource detectors will define the Collector internal telemetry's resource attributes, which is what the OpAMP extension surfaces.

But I share your concern about divergence. The SDK detectors are certainly not going away, so I agree it would be better to contribute to them and base Collector processors on them than the other way around.

[iblancasa]

I think this got some approvals. Maybe we can merge?

[evan-bradley]

Do we mark it as discouraged somewhere?

The argument I have heard is that if the Collector is overloaded, sending it's own telemetry to itself before processing makes the problem worse and risks that telemetry not making it to a backend in a timely manner. Therefore the recommendation is to remove that hop and send it to a separate Collector if necessary.