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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add apimachinery dependency #909

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Add apimachinery dependency #909

wants to merge 6 commits into from

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Feb 26, 2024

Everything exposed in the grafana apiserver will need an openapi representation compatible with https://github.com/kubernetes/kube-openapi, and top level resources will need to implement https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime#Object

We could try to keep the SDK k8s dependency free; but would then need to add wrappers/helpers somewhere and these would not have an easy integration point.

This PR adds the key dependencies:

require (
	k8s.io/apimachinery v0.29.2
	k8s.io/kube-openapi v0.0.0-20240220201932-37d671a357a5
)

This enables an apiserver to use results from the SDK directly without any special wrappers. The downside is bigger (but not tooooo much bigger) plugin builds

Before After
image image

~3mb for each executable 馃し馃徎 , but greatly simplifies our dependency cycles

With only kube-openapi, it adds ~1mb:
image

@@ -47,6 +47,15 @@ type Frame struct {
Meta *FrameMeta
}

func (f *Frame) DeepCopy() *Frame {
Copy link
Member Author

Choose a reason for hiding this comment

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

For now, frame does not need to be a root object, but implementing DeepCopy allows an upstream struct to include Frame and use standard k8s codegen tools

Copy link
Member Author

Choose a reason for hiding this comment

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

any suggestions for a better root path/name? "sdk?" once we are comfortable with this approach, this should live at the root

Copy link
Contributor

@tolzhabayev tolzhabayev left a comment

Choose a reason for hiding this comment

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

Please do not merge this until this is finished and approved by plugins platform - https://docs.google.com/document/d/1la4XW9BZ5XXyU91ZjbzEPE4jvPAyusE0dfJdsrCbbPI/edit

@andresmgot
Copy link
Contributor

We could try to keep the SDK k8s dependency free; but would then need to add wrappers/helpers somewhere and these would not have an easy integration point.

I understand why it makes sense to keep the openapi definition close to the struct (QueryDataResponse) and how this simplifies the dev flow but I am not sure if the extra size outweights the benefits, at least with the current state of the feature.

Extra 3MB per binary (6x3=18MB per plugin) can be noticeable for deployments with multiple plugins (e.g. ~180MB with 10 plugins), and there is no direct benefit for SDK consumers to have this today.

I believe the next, less painful, approach would be to have this wrapper in grafana/grafana, where the actual consumer of this is at the moment. We can re-evaluate this when there are multiple consumers of the openapi definition.

// QueryDataResponse contains the results from a QueryDataRequest.
// It is the return type of a QueryData call.
type QueryDataResponse struct {
metav1.TypeMeta `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

This might confuse plugin authors - what is it, do they need to populate/do anything with it etc? Creating a wrapper as @andresmgot syggests seems straightforward and allows us to keep this non-exposed for plugin devs for now. Interestingly, these objects including data frame might require multiple apiVersions eventually as well and how de we handle that? The wrapper can still live in the sdk though, maybe under experimental/api or something.

Interestingly we've discussed that Grafana should maybe have its own implementation of QueryDataResponse to allow deciding whether or not based on need to decode the responses into Go struct/data frames or not and return the encoded bytes (if json) directly to the client/browser. Earlier discussions have touched upon the idea that QueryDataResponse should be an interface to allow what just mentioned within Grafana.

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

4 participants