[processor/routing] Fix statement not eval in order

[Frapschen]

Description:

Rise a PR to change routes from type map[string]routingItem[E, K] to []routingItem[E, K] to fix statement not eval in order.

Link to tracking Issue: #34860

Testing:

Add three order relate tests to verify this change is work.

Documentation:

[atoulme]

was this change intended?

[jpkrohling]

To be honest, I find this test a bit confusing: the first two conditions will never be true for different reasons and the third is only resulting in true if the deletion happens based on the value of the key used in the where clause.

The statements could be just simple route statements?

route() where resource.attributes["__otel_enabled__"] == nil
route() where resource.attributes["__otel_enabled__"] == "true"
route() where resource.attributes["__otel_enabled__"] == "false" // this is the only one resulting in true

Or even better:

route() where resource.attributes["non-matching"] != nil // non-matching is not set, this is never true
route() where resource.attributes["non-matching"] == "true" // non-matching is not set, therefore, not "true"
route() where resource.attributes["matching"] == "true" // matches!

And in the main test code, have this attribute instead:

rl.Resource().Attributes().PutStr("matching", "true")

[Frapschen]

@jpkrohling The test is designed to run 5 times, and I expected it to produce the same result each time. This is how expected statements work. If the statements are executed randomly (map[string]routingItem[E, K]), they can't yield consistent results.

BWT, the statements is from the issue's description. It can demonstrate that statements are executed in order or randomly will affect the routing results. so I used it in the test.

[jpkrohling]

Running them 5 times is fine, my concern is about the readability of the test. While the statements on the issue description were enough to reproduce the problem, the tests should be easily understandable to our future selves without having to come back to this issue to get the full picture, IMO.

[jpkrohling]

I wonder what are the performance implications of this change. This code is in the hot path. If you are using this change in production already, are you able to provide some insights here?

[Frapschen]

I haven't tested it in production yet, but according to the changed code, it does not break the loop behavior. Whether routes is a map or a slice, it always loops through all the elements in the routes. So, I would say it will not affect the performance negatively.

[jpkrohling]

I'm not concerned about the behavior, I'm concerned about the performance. I just double-checked, and this doesn't seem like it's in the hot path, called only on the Start path. In any case, it would make me more comfortable if we had a benchmark comparing how it was before and how it is now.

[github-actions]

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

[Frapschen]

sorry for the delay @jpkrohling PTAL

[github-actions]

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

[codeboten]

Pinging @jpkrohling please confirm whether your comments have been addressed

[github-actions]

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

[jpkrohling]

The concerns about the readability of the tests weren't addressed yet, and tests for metrics and traces are missing.

[github-actions]

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