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

refactor: move init code from main to instrumentor package #2445

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

RonFed
Copy link
Collaborator

@RonFed RonFed commented Feb 14, 2025

This PR mainly refactor the instrumentor main function, and extracts it's code to the instrumentor package.

  • instrumentor.New initlizes the manager and all its controllers and webhooks.
  • instrumentor.Run launches the runnables including the manager itself.

The telemetry reporter was adjusted to accept a context so we can cancel it once the instrumentor shuts down and exit gracefully.

@@ -83,5 +83,19 @@ func SetupWithManager(mgr ctrl.Manager) error {
return err
}

err = builder.
Copy link
Contributor

Choose a reason for hiding this comment

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

this webhook is used by startlangdetection and deleteinstrumentationconfig controllers, so it can be set on the main instrumentor manager like it originally was

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.
Also added a comment about this, that we are planning to combine this controllers and the webhook can move into the unified controller once we do that. PTAL

@RonFed RonFed force-pushed the refactor_instrumentor_main branch from e6fe3cb to 129b603 Compare February 14, 2025 16:19
@RonFed RonFed marked this pull request as ready for review February 14, 2025 16:22
@RonFed RonFed merged commit 657ab4a into odigos-io:main Feb 16, 2025
44 of 45 checks passed
@RonFed RonFed deleted the refactor_instrumentor_main branch February 16, 2025 09:29
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.

3 participants