Skip to content

Plugin Support #952

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Plugin Support #952

wants to merge 19 commits into from

Conversation

tconley1428
Copy link
Contributor

@tconley1428 tconley1428 commented Jul 9, 2025

What was changed

Added a new Plugin concept for configuring clients and workers

Why?

Many users need to do a common set of configurations across multiple locations, or provide such a configuration set to others.

Checklist

  1. Closes [Feature Request] Plugins to support controlling multiple configuration points at once #950

  2. How was this tested:
    New set of tests in test_plugins.py

  3. Any docs updates needed?
    Readme is updated, docs will be needed.

Copy link
Contributor

@dandavison dandavison left a comment

Choose a reason for hiding this comment

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

This is great.

I spent some time seeing if we could improve any of the names in the interface.

What I'd like to suggest there is: instead of on_create_client and on_create_worker, how about

def client_config(self, config: ClientConfig) -> ClientConfig

def worker_config(self, config: WorkerConfig) -> WorkerConfig

An alternative that was mentioned on slack was configure_client() / configure_worker(). That could work too, but it kind of suggests that the method gets to instantiate or mutate a client/worker. I like the simplicity of client_config(self, config: ClientConfig) -> ClientConfig: it makes it clear that your job in that method is to return the config you want, nothing less and nothing more.

on_create_client and on_create_worker don't work as well because their names suggest an event that occurs after or during instantiation of the client/worker, which is awkward seeing as the method is a pure transformation of a config struct and so is clearly happening before instantiation.

Other than that, it took me a bit of time to understand why the multiple inheritance made sense, and why it made sense to expose the "service client" rather than just "connect client", but I think this all makes sense and is very natural given the SDK's existing public interceptor API.

@cretz
Copy link
Member

cretz commented Jul 16, 2025

Overall agree w/ @dandavison

An alternative that was mentioned on slack was configure_client() / configure_worker(). That could work too, but it kind of suggests that the method gets to instantiate or mutate a client/worker

I think it suggests the method gets to configure the client/worker, not instantiate or mutate it. I think it's clearer than the getter-looking ones that are missing the verb.

@dandavison
Copy link
Contributor

configure_client() / configure_worker()

Yes I think I've changed my mind, I prefer these also.

@tconley1428 tconley1428 marked this pull request as ready for review July 16, 2025 16:15
@tconley1428 tconley1428 requested a review from a team as a code owner July 16, 2025 16:15
@tconley1428 tconley1428 changed the title Initial rough framework for plugins Plugin Support Jul 16, 2025
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

LGTM, nothing blocking

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.

[Feature Request] Plugins to support controlling multiple configuration points at once
4 participants