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.

Sign up for GitHub

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

Jump to bottom

[connector/routing] Supports matching the statement only once. #28888

Merged
merged 10 commits into from
Dec 14, 2023
Merged

[connector/routing] Supports matching the statement only once. #28888

merged 10 commits into from
Dec 14, 2023

Conversation

huange7
Copy link
Contributor

@huange7 huange7 commented Nov 4, 2023 •
edited by songy23
Loading

Description:

Adding a feature: routing connector supports matching the statement only once

Link to tracking Issue: #26353

Testing:
Basic tests included for the functionality.

Documentation:

@huange7 huange7 requested a review from a team November 4, 2023 06:16
@github-actions github-actions bot requested a review from mwear November 4, 2023 06:16
@huange7 huange7 mentioned this pull request Nov 4, 2023
Closed
songy23
songy23 reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix linter: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/6753227844/job/18405547771?pr=28888

@huange7 huange7 requested a review from songy23 November 6, 2023 16:02
crobert-1
crobert-1 requested changes Nov 13, 2023
View reviewed changes
connector/routingconnector/README.md Outdated Show resolved Hide resolved
connector/routingconnector/README.md Outdated Show resolved Hide resolved
connector/routingconnector/README.md Outdated Show resolved Hide resolved
connector/routingconnector/config.go Outdated Show resolved Hide resolved
connector/routingconnector/config.go Outdated Show resolved Hide resolved
connector/routingconnector/router.go Show resolved Hide resolved
djaglowski
djaglowski requested changes Nov 27, 2023
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove routingItem.order field.

@huange7
Copy link
Contributor Author

huange7 commented Nov 28, 2023

Please remove routingItem.order field.

done

crobert-1
crobert-1 reviewed Nov 28, 2023
View reviewed changes
connector/routingconnector/README.md Outdated Show resolved Hide resolved
connector/routingconnector/README.md Outdated Show resolved Hide resolved
connector/routingconnector/config.go Show resolved Hide resolved
connector/routingconnector/traces_test.go Show resolved Hide resolved
@djaglowski
Copy link
Member

Related question which can be a separate issue. Is the other (match multiple routes) approach implemented correctly? I am wondering if we are not properly fanning out the data.

For example, if we match routes 1 and 2, we cannot send the same copy of the data to both consumers. We need to use a fanout. In other words, we should build a consumer which contains all the pipelines in both 1 and 2, and send to that.

@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Dec 14, 2023
@huange7
Copy link
Contributor Author

huange7 commented Dec 14, 2023

The PR hasn't been merged yet. Is there anything else I need to do?

@djaglowski djaglowski mentioned this pull request Dec 14, 2023
Open
@djaglowski
Copy link
Member

I've opened a new issue to track my suggestions related to matching multiple routes.

@crobert-1, would you mind reviewing the open conversations and resolving as appropriate.

@huange7, anything else you need from us here other than another review?

@djaglowski djaglowski removed the Stale label Dec 14, 2023
@huange7
Copy link
Contributor Author

huange7 commented Dec 14, 2023

@huange7, anything else you need from us here other than another review?

Thank you very much for your support. I don't have any other questions.

@crobert-1
Copy link
Member

All of my concerns have been addressed, but I don't have permission to resolve the open conversations. This may be because I don't have write access in contrib, but maybe I'm missing something.

@djaglowski
Copy link
Member

@huange7 I think we're good to go as soon as the conflict is resolved and CI passes

@djaglowski djaglowski merged commit b4563f3 into open-telemetry:main Dec 14, 2023
81 of 85 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 14, 2023
@huange7
Copy link
Contributor Author

huange7 commented Dec 15, 2023

@huange7 I think we're good to go as soon as the conflict is resolved and CI passes

Thanks. @djaglowski

cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jan 10, 2024
@huange7 @djaglowski @cparkins
[connector/routing] Supports matching the statement only once. (open-…
9def75b 
…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
Reviewers

@crobert-1 crobert-1 crobert-1 approved these changes

@djaglowski djaglowski djaglowski approved these changes

@jpkrohling jpkrohling Awaiting requested review from jpkrohling jpkrohling is a code owner

@mwear mwear Awaiting requested review from mwear mwear is a code owner

@songy23 songy23 Awaiting requested review from songy23

Assignees

@songy23 songy23

Labels
connector/routing
Projects
None yet
Milestone
v0.92.0
Development

Successfully merging this pull request may close these issues.
4 participants
@huange7 @djaglowski @crobert-1 @songy23