Introduce Tenant ID field to Plugin Context #669
Conversation
wbrowne
requested review from
andresmgot,
xnyo and
marefr
and removed request for
a team
There was a problem hiding this comment.
Thinking about this, from a protocol perspective, the missing information that you will need is probably just a tenantID.
Whoever is handling a cache (or similar) is the one that should generatee a valid key (as it's done here with pluginID + OrgID for example). It's a bit weird that the client (plugin) is setting a key that's only used in the server (grafana), isn't it?
Tend to agree, but it only make sense in a Grafana Cloud context :) That might be fine, but could also confuse SDK consumers. Guess cache key could confuse as well, but maybe not as much 🤷 I don't have any strong opinions really and open for any suggestion |
Sorry for the delay on this - also no strong opinions on this, but I feel less inclined towards using the term "tenantID" as it might give the wrong impression at this early stage.
Just to correct, Grafana is the client in this case, and the plugin is the server. Given this relationship, and the plugin acting as virtually an extension of Grafana, it makes sense to me that Grafana can dictate control over how a plugin's resources are managed, when it is the one managing and orchestrating across all plugins 🤷 |
Good point. But at the same time it's not like something we can change it a later stage when changing the protobuf definition so something we have to live with :) |
IMO I think that's fine if we mark it as "experimental"/optional/not-in-use
I am not sure I agree with this, even if Grafana is the orchestrator, is setting implementation details of things that doesn't manage (if I understood this correctly). If both use this
This is what I think too, it's probably going to remain however we set it. If "tenantID" is too specific / potentially incorrect, what about "UID"? That would make also more sense for a field that identifies a plugin + instance and it could be helpful in Grafana for things like telemetry. In any case, it's just a naming thing so feel free to go ahead with whatever you think is best (you know better :P) Naming is hard 😅 |
I think you're right and I'll stick with what I originally had in mind. Let's go with simplicity 🥇 |
wbrowne
changed the title
There was a problem hiding this comment.
It's a bit weird that the client (plugin) is setting a key that's only used in the server (grafana), isn't it?
I think its weird if you just focus on the cache generation part. Any multi tenant system pass some kind of unique identifier per tenant used by other system to distinguished them. Ex ops.grafana.net and bergquist.grafana.net
In this case used to generate different cache keys when we want to separate them. Other caches in the SDK might not use the tenantID.
@bergquist, @andresmgot's comment probably makes more sense with the context prior to to this commit in case you missed it 9fc02a0. Which would have meant that Grafana controls specifically what the entire cache key is - we did talk about this before and either way works, but this latest change feels like a simpler step for now if we want to move this direction. |
|
@wbrowne thanks for clearing that up! :) @andresmgot sorry for the confusion. |
|
Closing in favour of #676 |
What this PR does / why we need it:
Adds a new tenant ID field as part of the plugin request. This can be immediately used as supplementary info to the instance cache key for instance management
Special notes for your reviewer:
Related refactor which will eventually use this work