feat(routingprocessor): allow routing for all signals

[pmalek-sumo]

Description: routingprocessor: allow routing for all signals

Link to tracking Issue: N/A

Testing: Added relevant unit tests

Documentation: Changed README.md

[jpkrohling]

Looks great! Can you improve the test coverage? There are a few places with no coverage, like most of the router.RouteTraces. Also, there's some duplication in the router, with pretty much the same code for all signals, except for actually building the pdata. Do you think we can make it generic?

[pmalek-sumo]

Looks great! Can you improve the test coverage? There are a few places with no coverage, like most of the router.RouteTraces.

I'll take care of the coverage and the PR comments when I have a spare moment.

Also, there's some duplication in the router, with pretty much the same code for all signals, except for actually building the pdata. Do you think we can make it generic?

I was thinking about this and I couldn't come up with a proper approach to make it generic since there's no real interface overlap between the top level signal structs (pdata.Metrics, pdata.Logs and pdata.Traces) and/or the exporters.
I'm open to suggestions on this.

I'll try to figure out a way to make small routing related bits common to all of those but I can't promise much

[jpkrohling]

I'll try to figure out a way to make small routing related bits common to all of those but I can't promise much

Thank you, that's more than I can ask for!

[pmalek-sumo]

I'll try to figure out a way to make small routing related bits common to all of those but I can't promise much

Ok, so I thought about this and without resorting to using interface{} I don't believe we can (easily) solve this.

For instance

	resSpansSlice := tr.ResourceSpans()
	for i := 0; i < resSpansSlice.Len(); i++ {
		resSpans := resSpansSlice.At(i)
...

I could define an interface that that would a "generic for loop" i.e. something like:

type Looper interface {
	Len() int
	At(int) pdata.ResourceSpans <- this is the problematic bit
}

but then I cannot make it generic because the At(int) function in all the data structures (pdata.Traces, pdata.Metrics and pdata.Logs) are of different signatures, returning structs not interfaces.

Same applies to other places.

Resorting to interface{} could perhaps make it possible in some places but it would be more ugly code (albeit with less duplication).

Something similar applies to slices of interfaces e.g. where I return component.TracesExporter etc. Making it "generic" in some places i.e. returning component.Exporter could make sense but returning arrays of those in:

type routedTraces struct {
	traces    pdata.Traces
	exporters []component.Exporter <- as of now this is []component.TracesExporter
}

wouldn't work because one cannot assign:

	e := []component.Exporter{}
	te := []component.TracesExporter{}
	e = te

cannot use te (variable of type []component.TracesExporter) as []component.Exporter value in assignment

(in order to make the last bit work we'd need to loop and append each entry 😞 )

Anyway: I'm not saying this is impossible but it's tricky to make it pretty in this particular example.

[pmalek-sumo]

Looks great! Can you improve the test coverage? There are a few places with no coverage, like most of the router.RouteTraces.

I'll take care of the coverage and the PR comments when I have a spare moment.

I've added more tests @jpkrohling, PTAL

[jpkrohling]

Anyway: I'm not saying this is impossible but it's tricky to make it pretty in this particular example.

Thanks for investigating! Perhaps we could start an experiment in the future, post-GA, seeing how we can use Go generics to simplify the API.

[jpkrohling]

LGTM, just a question about why you are having two data points being consumed, as I don't remember it having any influence on the code under test.

[jpkrohling]

LGTM!

[bogdandrutu]

@jpkrohling lint errors, please add the "ready-to-merge" label when ready for real :)

[pmalek-sumo]

Ok, I see. The Validate() is called at the collector level, hence it's the errors it returns are not reflected in UTs: https://github.com/open-telemetry/opentelemetry-collector/blob/f85f13e27b3f80fd317a52fa366348b8405dbb97/service/collector.go#L166

I've changed the code so that newProcessor doesn't explicitly call Validate() which also simplified a handful of tests.

@jpkrohling PTAL.