[xscraperhelper] init package

[florianl]

Description

Along with scraper/xscraper this package is needed to enable OTel Profiling signal receivers, such as receiver/pprofreceiver.

Link to tracking issue

Fixes #

Testing

Documentation

[florianl]

This part is auto-generated by make generate-gh-issue-templates. It looks like these components are missing so far.

[codspeed-hq]

CodSpeed Performance Report

Merging #14235 will improve performances by 39.62%

_{Comparing florianl:xscraperhelper (9e1249b) with main (8f51a17)}

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

⚡ 7 improvements
✅ 52 untouched
⏩ 20 skipped¹

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	BenchmarkTraceSizeSpanCount	60 ns	46 ns	+30.43%
⚡	BenchmarkTracesToProto2k	96 µs	69.7 µs	+37.66%
⚡	BenchmarkTraceSizeBytes	430.5 µs	317.2 µs	+35.71%
⚡	with_a_duplicate_value	60 ns	46 ns	+30.43%
⚡	BenchmarkTracesMarshalJSON	6 µs	4.7 µs	+26.05%
⚡	BenchmarkProfilesToProto	1.7 µs	1.3 µs	+29.26%
⚡	BenchmarkLogsToProto2k	63.2 µs	45.3 µs	+39.62%

Footnotes

1. 20 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. ↩

[codecov]

Codecov Report

❌ Patch coverage is 94.90196% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.18%. Comparing base (8f51a17) to head (9e1249b).

Files with missing lines	Patch %	Lines
scraper/scraperhelper/xscraperhelper/controller.go	89.74%	2 Missing and 2 partials ⚠️
...er/scraperhelper/internal/controller/controller.go	95.00%	2 Missing and 1 partial ⚠️
...craper/scraperhelper/internal/testhelper/helper.go	94.44%	2 Missing ⚠️
...perhelper/internal/metadata/generated_telemetry.go	93.33%	1 Missing and 1 partial ⚠️
...raper/scraperhelper/xscraperhelper/obs_profiles.go	94.44%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (94.90%) 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   #14235      +/-   ##
==========================================
+ Coverage   92.17%   92.18%   +0.01%     
==========================================
  Files         668      674       +6     
  Lines       41510    41679     +169     
==========================================
+ Hits        38262    38422     +160     
- Misses       2215     2220       +5     
- Partials     1033     1037       +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]

It looks like xscraperhelper duplicates a lot of code from scraperhelper, which is probably why the PR is so large. This is not ideal, since it makes maintenance a lot more difficult (any change in scraperhelper would have to be copied over to xscraperhelper). I think a better approach would be to move the "generic" components used in all signals into scraperhelper/internal/xxx packages, so that you can reuse them in scraperhelper/xscraperhelper without duplicating their code.

[florianl]

@jade-guiton-dd with 92bc761 I have started to deduplicate code. But the win of deduplication is limited.

While it seems that a good portion of code is duplicate, they differ in essential elements, like receiver vs xreceiver, consumer vs xconsumer, scraper vs xscraper. Due to these fundamental differences, there seems to be a huge overlapp but it is actually not.

[florianl]

Good catch with the controller - I moved it with 71b753d.

[florianl]

@jade-guiton-dd I'm about to revert 71b753d - the move of parts of controller into a dedicated shared package.
The reason is, that this is a major breaking change for all components, that use scraperhelper, as ControllerConfig is an essential part of the scraperhelper API - e.g. it is used in NewLogsController(), NewMetricsController() and other major parts of the API of scraperhelper.

In other parts of the OTel collector, like xreceiver, xscraper, xconsumer, xprocessor and others, the code duplication was not a problem. The x-prefixed packages are seen to be temporarily until the profiling Signal becomes more stable, while keeping the rest of OTel collector stable. Is the situation here different?

[jade-guiton-dd]

I'm about to revert 71b753d - the move of parts of controller into a dedicated shared package.
The reason is, that this is a major breaking change for all components, that use scraperhelper, as ControllerConfig is an essential part of the scraperhelper API - e.g. it is used in NewLogsController(), NewMetricsController() and other major parts of the API of scraperhelper.

I think moving exported types/functions like ControllerConfig to an internal package is unnecessary, xscraperhelper should be able to import them directly from the scraperhelper package.

In other parts of the OTel collector, like xreceiver, xscraper, xconsumer, xprocessor and others, the code duplication was not a problem. The x-prefixed packages are seen to be temporarily until the profiling Signal becomes more stable, while keeping the rest of OTel collector stable. Is the situation here different?

I'm not sure. I would say those packages contain less functional code than a helper like xscraperhelper, ie. they're mostly just interfaces, where relatively little can be reused, so I do think it's a bit of a different situation. But I agree that this is subjective and that the "temporary" nature of the x packages may warrant more leniency in terms of code duplication. I will ask other maintainers what they think.

[jade-guiton-dd]

It sounds like for the other x*helper packages, there were already internal packages to take advantage of for code reuse, so I won't insist too hard on it. If making a version of 71b753d that just imports ControllerConfig from the main scraperhelper package doesn't work out or is too much trouble, I'll review the PR as-is and ignore the duplicated code.

[florianl]

@jade-guiton-dd with using scraperhelper in xscraperhelper I was able to remove the duplicate code around ControllerConfig.

For type controller[T component.Component] struct{} and its methods I wasn't able to do so. I didn't want to make controller and its methods part of the public API of scraperhelper and moving it into an internal package creates a circular dependency, as controller depends on ControllerConfig of scraperhelper.

Hope this approach works for you. WDYT?

[jade-guiton-dd]

The solution here would be to move ControllerConfig to an internal package and reexport it in the main package, which is a common pattern when dealing with x packages, see for example here in the exporterhelper. This would allow you to move controller there as well without creating a circular dependency.

If that still doesn't work, I won't insist further on deduplication, but I'd rather avoid maintenance hell later, even if we've been more lenient for x packages in the past.

[florianl]

The solution here would be to move ControllerConfig to an internal package a

This option will not work, as ControllerConfig is an argument to the API of scraperhelper. If ControllerConfig is in a internal subpackage, it will break uses like in OTel Collector contrib as the use of internal from third parties is not allowed.

Do you want to see a non-internal subpackage, like scraperhelper/controller? This would allow external users to use it. I didn't went this route, as it feels odd to have a public controller package in OTel collector, that is for scraperhelper.

[jade-guiton-dd]

This option will not work, as ControllerConfig is an argument to the API of scraperhelper. If ControllerConfig is in a internal subpackage, it will break uses like in OTel Collector contrib as the use of internal from third parties is not allowed.

I don't understand, would it not work if scraperhelper re-exports ControllerConfig from the internal package?

[florianl]

I have updated the PR, moved the code into an internal package and used type aliasing as well as a function wrapper, to not break the API.

[jade-guiton-dd]

It's difficult giving a thorough review on so much code, but after comparing with the old code it seems cogent. I just have a few questions and comments.

[jade-guiton-dd]

I saw the logs/metrics versions of this file have a call to assertReceiverSpan(t, spans) here. Was it removed on purpose because of the missing receiverhelper telemetry, or was it a mistake?

[florianl]

scrapeProfiles() does not emit this data atm therefore assertReceiverSpan(t, spans) was removed.

Currently scrapeProfiles() can not generate this data, as #14251 is still not merged.

As #14251 is in another package and independent from this PR, I didn't want to mix things and created a dedicated PR.