[chore] Move back receiver definitions, make profile embed receiver

[bogdandrutu]

No description provided.

[dmathieu]

I don't have the context around this change, but this approach is what we've been doing for every other component that needs profiles.
I'm also not sure this can work, as receiver.NewFactory needs to be able to handle profiles. With this change, it can't, since it returns an interface that doesn't implement it.

cc @mx-psi (who is OOO this week)

[bogdandrutu]

@dmathieu the current implementation is very bad because the public "soon to be stable" receiver.Factory has an unstable API, so current solution definitely is not working as expected. See https://pkg.go.dev/go.opentelemetry.io/collector/receiver#Factory the press the "internal.Factory" to read the definition.

I'm also not sure this can work, as receiver.NewFactory needs to be able to handle profiles. With this change, it can't, since it returns an interface that doesn't implement it.

Why?

[dmathieu]

Why?

In the service, the receiver builder holds a list of receivers:

opentelemetry-collector/service/internal/builders/receiver.go

Lines 20 to 23 in 4709346

	type ReceiverBuilder struct {
	cfgs map[component.ID]component.Config
	factories map[component.Type]receiver.Factory
	}

But it also needs to be able to treat it as a profiles receiver, if we wish to create one:
https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/internal/builders/receiver.go#L104

Hence the current implementation in main. The full (with profiles) implementation is hidden in internal. The one in the receiver packages only includes the stable methods.
Anything unstable is in receiverprofiles, but it's still the same implementation, so the service can use it.

Similarly, we need it in the OTLP receiver, to be able to create the receiver and have it handle profiles:

opentelemetry-collector/receiver/otlpreceiver/factory.go

Lines 29 to 38 in 2be0dfb

	func NewFactory() receiver.Factory {
	return receiver.NewFactory(
	metadata.Type,
	createDefaultConfig,
	receiver.WithTraces(createTraces, metadata.TracesStability),
	receiver.WithMetrics(createMetrics, metadata.MetricsStability),
	receiver.WithLogs(createLog, metadata.LogsStability),
	receiverprofiles.WithProfiles(createProfiles, metadata.ProfilesStability),
	)
	}

Reverting this would have a much bigger impact that this PR, and would likely require reverting and rethinking most of the profiling work I've been doing those past 3 months.

This approach was decided in https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/rfcs/experimental-profiling.md (#10375)

[bogdandrutu]

@dmathieu look at the PR and you see that it works. I am not sure where all these are coming from. You can see that the service can still create Profiles.

PR passes all tests, if you have good test coverage (which I expect you should) everything still works and we don't expose the profiles on a stable package.

[bogdandrutu]

The full (with profiles) implementation is hidden in internal. The one in the receiver packages only includes the stable methods.

This is not true, go ahead and check that f := receiver.NewFactory(...); f.CreateProfilesReceiver(...) will compile.

Update: Even f.ProfilesReceiverStability()

[bogdandrutu]

Reverting this would have a much bigger impact that this PR, and would likely require reverting and rethinking most of the profiling work I've been doing those past 3 months.

All you need to do is to create the factory using receiverprofiles.NewFactory when the component supports profiles, that is the only change required.

[dmathieu]

You're right, sorry, it does work.
IMHO, there are still a couple problems with your approach. The main one being that folks now have to change the module they use for the receiver even for traces and metrics, as soon as they want to support traces, which isn't a great approach.

It's unlikely to happen, but with this approach, we also can't have 2 unstable signals at the same time (and use them together).
I've taken a stab at an alternative proposal, which I believe solves the stability problem, and is cleaner: #11270.

[bogdandrutu]

I've taken a stab at an alternative proposal, which I believe solves the stability problem, and is cleaner: #11270.

I have 2 problems I am looking to solve:

• Stability, which indeed is solved by your proposal;
• GoDoc, which does not work nice when using type aliasing with internal. See https://pkg.go.dev/go.opentelemetry.io/collector/receiver#pkg-types, which causes lots of issues for the developers to read our documentation.

It's unlikely to happen, but with this approach, we also can't have 2 unstable signals at the same time (and use them together).

We can, we just need to call that package "receiverexperimental" or something like that instead of the "receiverprofiles".

The main one being that folks now have to change the module they use for the receiver even for traces and metrics, as soon as they want to support traces, which isn't a great approach.

I think this is a minimal problem in my opinion (we are just looking for things that are not ideal here), since anyway they have to use the "experimental" module anyway. I care way more about our "stable" developer experience than the "experimental" developer experience, if you need experimental it is ok to do more work.

[bogdandrutu]

receiver.NewFactory needs to be able to handle profiles.

This is another bad design, see https://www.hyrumslaw.com

[bogdandrutu]

@open-telemetry/collector-approvers please review this, since @dmathieu work is not affected and this is a pure collector design I am sure we can move forward with this solution that has clear advantages vs existing or proposed alternatives:

• Does not expose unstable API;
• Does clean the godoc
• Does not have ensure profiles are always present see https://www.hyrumslaw.com/ for why that can be considered public API.

[dmitryax]

This solution looks good to me

[codecov]

Codecov Report

Attention: Patch coverage is 82.35294% with 15 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@d6f568d). Learn more about missing BASE report.
Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
receiver/receiverprofiles/profiles.go	60.00%	13 Missing and 1 partial ⚠️
receiver/receiver.go	97.56%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11254   +/-   ##
=======================================
  Coverage        ?   91.37%           
=======================================
  Files           ?      423           
  Lines           ?    20204           
  Branches        ?        0           
=======================================
  Hits            ?    18462           
  Misses          ?     1358           
  Partials        ?      384           

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