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

Remove unnecessary Session, one per ConfigSource anyway #3797

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

bogdandrutu
Copy link
Member

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

Copy link
Contributor

@pjanotti pjanotti left a 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.

config/internal/configsource/manager.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dashpole dashpole left a 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

@github-actions
Copy link
Contributor

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 <[email protected]>
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@bogdandrutu bogdandrutu merged commit 6e5beaf into open-telemetry:main Aug 19, 2021
@bogdandrutu bogdandrutu deleted the rmsession branch August 19, 2021 17:43
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.

None yet

5 participants