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

Use tenant ID from incoming gRPC meta for instance caching #676

Merged
merged 9 commits into from
May 23, 2023

Conversation

wbrowne
Copy link
Member

@wbrowne wbrowne commented May 2, 2023

What this PR does / why we need it:
Adds a new tenant package which is responsible for reading tenant information for each incoming plugin request. This information will be attached to the Go context, to be used as supplementary info for instance management caching.

This approach is documented as option 2 in this doc.

Special notes for your reviewer:
The InstanceManager API requires a new context.Context argument for the relevant information to be propagated, which is the primary breaking change. This has an effect on core plugins and a number of community plugins.

Breakdown of the changes:

# github.com/grafana/grafana-plugin-sdk-go/backend/instancemgmt
## incompatible changes
InstanceManager.Do: changed from func(github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext, InstanceCallbackFunc) error to func(context.Context, github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext, InstanceCallbackFunc) error
InstanceManager.Get: changed from func(github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext) (Instance, error) to func(context.Context, github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext) (Instance, error)
InstanceProvider.GetKey: changed from func(github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext) (interface{}, error) to func(context.Context, github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext) (interface{}, error)
InstanceProvider.NeedsUpdate: changed from func(github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext, CachedInstance) bool to func(context.Context, github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext, CachedInstance) bool
InstanceProvider.NewInstance: changed from func(github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext) (Instance, error) to func(context.Context, github.com/grafana/grafana-plugin-sdk-go/backend.PluginContext) (Instance, error)

# github.com/grafana/grafana-plugin-sdk-go/backend/tenant
## compatible changes
package added

A follow up will come to likely remove instance management (where possible) from the public API, in favour of the auto instance management functionality which is already used as part of the primary entrypoints for both apps and data sources

@wbrowne wbrowne self-assigned this May 2, 2023
@wbrowne wbrowne marked this pull request as ready for review May 8, 2023 12:36
@wbrowne wbrowne requested a review from a team as a code owner May 8, 2023 12:36
@wbrowne wbrowne requested review from marefr and andresmgot and removed request for a team May 8, 2023 12:36
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@wbrowne wbrowne requested review from bergquist, xnyo and ryantxu May 9, 2023 16:13
backend/stream_adapter.go Outdated Show resolved Hide resolved
@wbrowne
Copy link
Member Author

wbrowne commented May 9, 2023

Side note @bergquist, had you considered what (if anything) would change about how we handle metrics based on tenant info https://github.com/grafana/grafana-plugin-sdk-go/blob/main/backend/diagnostics_adapter.go#L25

Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

LGTM, amazing work 🚀!

Regarding the discussion around tenant id in context vs plugin context, I don't have any strong opinions, but I definitely agree about removing manual instance management from the public api in favor of automatic instance management.

Also worth mentioning that PluginContext is a GRPC message, and adding tenant id over there would require changing the protobuf definition. Since we're not 100% sure yet how this deployment model will look like at the end, I personally think it's safer to have it in the grpc metadata for now, as that's much easier to remove/deprecate compared to protobuf.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

okay, let's go ahead with this if everyone agrees

@wbrowne wbrowne merged commit 2b4a9fb into main May 23, 2023
2 checks passed
@wbrowne wbrowne deleted the tenant-id-grpc-meta branch May 23, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants