-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[connector/routing] Use inferred contexts #44762
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
base: main
Are you sure you want to change the base?
[connector/routing] Use inferred contexts #44762
Conversation
| r.logger.Warn(fmt.Sprintf(`Statement %q already exists in the routing table, the route with target pipeline(s) %q will be ignored.`, item.Statement, exporters)) | ||
| // Without this continue, the duplicate's pipelines would overwrite the original | ||
| // route's consumer, contradicting the warning message above. | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This continue was missing previously, meaning that the loop would not comply with the the logger warn statement above r.logger.Warn(fmt.Sprintf(Statement %q already exists in the routing table, the route with target pipeline(s) %q will be ignored., item.Statement, exporters)).
I had to restructure the control flow here because adding the continue statement triggered the lint check to request the if c {} else { continue } be restructed as an if !c { continue} ...
| return err | ||
| } | ||
|
|
||
| // singleStatementConverter extracts a single parsed statement from the parser output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers, if this feels like overkill I'm fine to remove it FYI
| require.NoError(t, err) | ||
|
|
||
| statements := mockGetter{[]string{`set(attributes["bar"], "bar")`}} | ||
| _, err = pc.ParseStatementsWithContext("bar", statements, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no cases in the actual non-test codebase where this is set to true
| // exist in resource anyway, and non-ambiguous paths (like "body", "severity_text", "name") | ||
| // only exist in one context regardless of priority. That said, the order is sorted based on which | ||
| // events are most common in practice, hence 'span' is first. | ||
| priorities := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edmocosta do you have any advice for how to handle potential new types that get added to OTel in the future? I'm not nuts about this list sitting here, but it's no different than the previous case statements I suppose.
Description
This PR allows users to use OTTL context inference in the routing connector. This makes routing table entries like the one below functional:
Additionally, this PR fixes a minor bug where duplicate routing table entries were handled in the opposite manner as the error message indicated. The error message indicated that that the duplicate would be ignored, but in fact the original entry was replaced. I'm glad to extract this fix to a separate PR if that is preferred by reviewers.
Link to tracking issue
Fixes #38080
Testing
You can test the new routing features manually by downloading config.yaml which showcases the different features of this PR.
Then, run:
make otelcontribcol && bin/otelcontribcol_darwin_arm64 --config config.yamlTo send test input:
Documentation
New docs have been added to the README