-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Remove unnecessary Session, one per ConfigSource anyway #3797
Conversation
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.
LGTM.
Just for the benefit if someone reaches this PR later: initial idea was to create the config source for the life of the collector in that context it appeared to have value to separate the config source from the session. In practice, the separation provided little benefit especially with the manager creating the config sources each time the configuration was reloaded.
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.
lgtm after open comments are addressed
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR makes the ConfigSource equivalent with the Session, and relies on the fact that when created ConfigSource does what NewSession was doing. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
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.
LGTM
This PR makes the ConfigSource equivalent with the Session, and
relies on the fact that when created ConfigSource does what
NewSession was doing.
Depends on #3792