Skip to content

Conversation

@rajkumar-rangaraj
Copy link
Member

@rajkumar-rangaraj rajkumar-rangaraj commented Apr 4, 2023

Why

Fixes #2392

What

This PR adds a rule engine to the StartupHook. The rule engine is designed to validate all OpenTelemetry assemblies and ensures that it backs off in unsupported scenarios instead of crashing. It validates all the rules and logs error to logger.

Implemented one of the rule, want to follow up other rules in a follow up PR. Verified with manual test, it works as expected. CI results should confirm that it does not change behavior.

Follow-up items

  • Add implementation of other rules
  • Add tests to validate rules
  • Update changelog

Tests

Checklist

  • [ ] CHANGELOG.md is updated.
  • [ ] Documentation is updated.
  • [ ] New features are covered by tests.

@rajkumar-rangaraj rajkumar-rangaraj requested a review from a team April 4, 2023 05:05
Copy link
Member

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

In general looks good.
It will be great to have at least scaffolding/smoke tests in this PR but I will not block this.

Copy link
Contributor

@RassK RassK left a comment

Choose a reason for hiding this comment

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

Seems good. +1 to the thing that @Kielek mentioned.
Not applying to this PR but DS rule and Instrumentation assembly rule should have integration tests to be sure that these execute properly and application is not crashing before.

Turn off flag is also needed (I think in a new PR), so who has staging environment can optimize loading speed in production.

@rajkumar-rangaraj
Copy link
Member Author

Seems good. +1 to the thing that @Kielek mentioned. Not applying to this PR but DS rule and Instrumentation assembly rule should have integration tests to be sure that these execute properly and application is not crashing before.

Turn off flag is also needed (I think in a new PR), so who has staging environment can optimize loading speed in production.

  • Very good call out, in the follow up PRs add integration tests to ensure that we are not crashing with lower version of libraries.
  • Along with turn-off flag, plan is to make the RuleEngine smart to execute once for an app. Yet to design this part.

@rajkumar-rangaraj
Copy link
Member Author

rajkumar-rangaraj commented Apr 4, 2023

I want to keep this PR simple. Adding the tests makes it complex. My next PRs will address the test gaps for the RuleEngine.

@pjanotti
Copy link
Contributor

pjanotti commented Apr 4, 2023

Merging it @rajkumar-rangaraj ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Add Rule Engine to StartupHook

4 participants