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

[extension/jaegerremotesampling] Tie in the strategy storages #8818

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

jpkrohling
Copy link
Member

This change adds support for the strategy stores, previously referenced as client config managers.

This implements both the local file strategy store and the remote (gRPC) store.

Fixes #6695

Signed-off-by: Juraci Paixão Kröhling [email protected]

@jpkrohling jpkrohling requested review from a team and dashpole March 23, 2022 19:25
@jpkrohling jpkrohling changed the title Tie in the strategy storages [extension/jaegerremotesampling] Tie in the strategy storages Mar 23, 2022
@jpkrohling
Copy link
Member Author

@dashpole, once this is reviewed, I'll rebase and take care of the conflicts.

require.NoError(t, e.Start(context.Background(), componenttest.NewNopHost()))

// test and verify
assert.NoError(t, e.Shutdown(context.Background()))
}

func TestFailedToStartHTTPServer(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test just not needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of: previously, I had the ability to provide an HTTP server, making it trivial to test a failure scenario. Now, I moved the entire initialization logic to the start function, so that an HTTP server is created there. Failure testing is now more complicated.

extension/jaegerremotesampling/go.mod Outdated Show resolved Hide resolved
@jpkrohling jpkrohling force-pushed the jpkrohling/issue6695 branch from d62885e to dbe3d57 Compare March 25, 2022 17:39
@jpkrohling
Copy link
Member Author

This is ready for another review round.

@jpkrohling
Copy link
Member Author

@dashpole, would you mind reviewing this once again?

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

@jpkrohling jpkrohling force-pushed the jpkrohling/issue6695 branch from dbe3d57 to 5a63410 Compare March 31, 2022 11:31
@jpkrohling jpkrohling force-pushed the jpkrohling/issue6695 branch from 5a63410 to 1660d6e Compare April 1, 2022 09:11
This change adds support for the strategy stores, previously referenced as client config managers.

This implements both the local file strategy store and the remote (gRPC) store.

Fixes #6695

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
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.

Tie in the client config manager proxy
4 participants