Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

processor/routing supports matching the statement only once. #26353

Closed
huange7 opened this issue Aug 31, 2023 · 18 comments
Closed

processor/routing supports matching the statement only once. #26353

huange7 opened this issue Aug 31, 2023 · 18 comments
Assignees
Labels
connector/routing enhancement New feature or request

Comments

@huange7
Copy link
Contributor

huange7 commented Aug 31, 2023

Component(s)

processor/routing

Is your feature request related to a problem? Please describe.

If multiple statements are configured in the configuration file, the routing processor will judge each statement. Whenever a match is made, data will be sent to the exporters defined in the configuration. Can we support interrupting the match after matching only once?

Describe the solution you'd like

Use a switch to allow the routing processor to match only once.

Describe alternatives you've considered

No response

Additional context

 routing:
    default_exporters:
      - jaeger
    error_mode: ignore
    table:
      - statement: route() where resource.attributes["service.name"] == "demo"
        exporters: [ logging ]
      - statement: route() where resource.attributes["service.name"] == "demo" and resource.attributes["token"] == "xxx"
        exporters: [ logging/2 ]

As shown in the configuration file above, I hope that some traces/metrics/logs will be sent to logging exporter if they only match the service.name attribute, and if they match both the service.name and the token, they will be sent to logging/2 exporter. This is possible in practice.

If this seems reasonable, I sincerely hope that I can make some contributions.

@huange7 huange7 added enhancement New feature or request needs triage New item requiring triage labels Aug 31, 2023
@github-actions github-actions bot added the processor/routing Routing processor label Aug 31, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

Hello @huange7, would it be possible to simply make your routing statements more specific to accomplish this functionality?

It sounds like your goal is to match only the most specific routing statement possible, and ignore any statement more general. If this is correct, we can simply update the general statement in your configuration to be more specific, so the match isn't overlapping anymore (a given log/metric/trace will then only match one statement). In your example, you could simply do the following:

 routing:
    default_exporters:
      - jaeger
    error_mode: ignore
    table:
      - statement: route() where resource.attributes["service.name"] == "demo" and resource.attributes["token"] != "xxx"
        exporters: [ logging ]
      - statement: route() where resource.attributes["service.name"] == "demo" and resource.attributes["token"] == "xxx"
        exporters: [ logging/2 ]

Apologies if I've misunderstood or missed anything here.

@huange7
Copy link
Contributor Author

huange7 commented Sep 1, 2023

would it be possible to simply make your routing statements more specific to accomplish this functionality?

@crobert-1 Thanks for your reply. This sounds feasible.

However, let's consider a scenario where there is a processor that adds data.index attribute as the first layer of routing, and a second processor that adds exporter.index attribute as the second layer of routing. In this case, we want to configure a default route for the first layer of routing (data.index), and then configure individual routes for each value of exporter.index.

In that case, the configuration file would look like this:

routing:
    default_exporters:
      - jaeger
    error_mode: ignore
    table:
      - statement: route() where resource.attributes["data.index"] == "1" and resource.attributes["exporter.index"] != "1" and resource.attributes["exporter.index"] != "2"
         exporters: [ logging ]
      - statement: route() where resource.attributes["data.index"] == "1" and resource.attributes["exporter.index"] == "1"
        exporters: [ logging/1 ]
      - statement: route() where resource.attributes["data.index"] == "1" and resource.attributes["exporter.index"] == "2"
        exporters: [ logging/2 ]

As the number of different exporter.index values increases, the configuration for the default route could become more complex and lengthy. It's important to consider how to maintain the readability and manageability of the configuration file in such cases.

@jpkrohling
Copy link
Member

Can you confirm the same applies to the connector? I'd be more inclined to accept this new feature there instead of the processor.

@huange7
Copy link
Contributor Author

huange7 commented Sep 5, 2023

Can you confirm the same applies to the connector? I'd be more inclined to accept this new feature there instead of the processor.

Hello, @jpkrohling. Do you mean to add this feature to the routing connector rather than the routing processor? If so, I can implement this feature in the connector.

@crobert-1
Copy link
Member

/label -needs-triage

@github-actions github-actions bot removed the needs triage New item requiring triage label Sep 5, 2023
@crobert-1
Copy link
Member

/label connector/routing -processor/routing

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Pinging code owners for connector/routing: @jpkrohling @mwear. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

I'm going to assign this to you @huange7, let me know if that's incorrect.

@mwear
Copy link
Member

mwear commented Sep 8, 2023

@huange7, I'm curious what your plans for solving this issue are. Are you planning on changing the behavior of the routing connector to only route to the first match or are you planning on adding additional functionality to instruct the router to stop after a match conditionally? If the latter, how do you see that working?

@huange7
Copy link
Contributor Author

huange7 commented Sep 11, 2023

hello, @crobert-1 @mwear

I will modify the routing connector to match only the first matching route, because currently, the mapping between statements and routing items is stored using a map, which results in the matching order not following the order in the configuration. In order to match the most precise route, should we consider introducing an order variable to control the matching order?

@mwear
Copy link
Member

mwear commented Sep 11, 2023

@huange7 As the routing connector is meant to eventually replace the routing processor, I think we need to continue to support the current behavior (routing to all matches) and add additional functionality to support routing to only the first match. I could see how adding an order or priority would help to sort the routing rules. This should only be needed for the match-first strategy though. @jpkrohling do you have any thoughts?

@huange7
Copy link
Contributor Author

huange7 commented Sep 12, 2023

@mwear We should consider having a switch to control the matching behavior of the routing connector. For instance, when the switch is turned on, it would match only once.

@jpkrohling
Copy link
Member

I will modify the routing connector to match only the first matching route

That's not acceptable, as it will impact current users that depend on routes matching different statements.

For instance, when the switch is turned on, it would match only once.

I agree, as long as the result is deterministic.

@huange7
Copy link
Contributor Author

huange7 commented Sep 12, 2023

That's not acceptable, as it will impact current users that depend on routes matching different statements.

@jpkrohling I apologize for my previous unclear description. I would like to add a switch for the connector, which will be used to control the matching behavior of the connector.

@djaglowski
Copy link
Member

I agree that a boolean setting on the connector makes sense. By default, it should route data to all matching routes (i.e. acts as a series of if statements). When the flag is set to true it will route only to the first match (acting as a switch statement).

@huange7
Copy link
Contributor Author

huange7 commented Nov 4, 2023

#28888 I have submitted a PR

djaglowski added a commit that referenced this issue Dec 14, 2023
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Adding a feature: routing connector supports matching the statement only
once

**Link to tracking Issue:** <Issue number if applicable> #26353

**Testing:** <Describe what testing was performed and which tests were
added.>
Basic tests included for the functionality.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Daniel Jaglowski <[email protected]>
@crobert-1
Copy link
Member

Resolved by #28888, thanks for your contribution @huange7!

cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
…telemetry#28888)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Adding a feature: routing connector supports matching the statement only
once

**Link to tracking Issue:** <Issue number if applicable> open-telemetry#26353

**Testing:** <Describe what testing was performed and which tests were
added.>
Basic tests included for the functionality.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Daniel Jaglowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector/routing enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants