Add lookup for additional metadata from JS plugins #2099

xtreme-vikram-yadav · 2021-03-03T15:15:48Z

What this PR does / why we need it:
It allows octant to extract out additional contextual data sent by the JS plugins.

Which issue(s) this PR fixes

Fixes Plugins can enrich calls to custom object stores with arbitrary metadata #2042

Special notes for your reviewer: There will be a separate PR to add similar support for Go plugins.

Related PR: vmware-archive/plugin-library-for-octant#8

xtreme-vikram-yadav · 2021-03-03T15:19:44Z

pkg/plugin/javascript/dashboard_delete.go

@@ -44,6 +44,16 @@ func (d *DashboardDelete) Call(ctx context.Context, vm *goja.Runtime) func(c goj
 		// This will never error since &key is a pointer to a type.
 		_ = vm.ExportTo(obj, &key)

+		if len(c.Arguments) > 1 {


This is needed to keep the metadata argument optional. I don't have strong opinions about this so let me know if anybody prefers this to always be passed with calls to the data store.

Javascript doesn't care one way or the other, so either way seems fine to me.

I was thinking of a scenario where octant users decide to use the older plugins with new octant release. This will not introduce any breaking change in that case

xtreme-vikram-yadav · 2021-03-03T15:20:40Z

pkg/plugin/javascript/dashboard_delete.go

+
+			_ = vm.ExportTo(metadataObj, &metadata)
+			for key, val := range metadata {
+				ctx = context.WithValue(ctx, key, val)


Seems like the context passed in here has a long life and adding context here keep increasing the memory imprint of the context. While it doesn't impact the correctness of the functionality, its growth with every execution of this code is concerning to me.

Example when i logged the context(I am passing datetime utc string from JS plugin in this example):

It started off with:
context : context.Background.WithCancel.WithValue(type log.key, val <not Stringer>).WithValue(type context.OctantContextKey, val <not Stringer>).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT)

After a while the context grew:

context : context.Background.WithCancel.WithValue(type log.key, val <not Stringer>).WithValue(type context.OctantContextKey, val <not Stringer>).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT)^C2021-03-02T21:03:19.268-0500```

Is this because we're iterating over all the keys of the metadata, but only deleting the last key?

It was my impression that context.WithValue creates a copy rather than mutating the existing instance, so because, here, the copied ctx object goes out of scope at the end of this method, I would not have thought a memory leak was possible.

If it is a long-lived object, how does it behave when multiple calls specify different values for a given key?

I should be using a different variable name than key here to avoid confusion

It's because i'm reusing the variable for the new context. I'm experimenting with a new variable for each function call to avoid this.

[vmware-archive#2042] Signed-off-by: Vikram Yadav <yvikram@vmware.com>

chinigo

A handful of questions but no objections.

chinigo · 2021-03-03T17:01:58Z

pkg/plugin/javascript/dashboard_delete.go

+
+			_ = vm.ExportTo(metadataObj, &metadata)
+			for key, val := range metadata {
+				ctx = context.WithValue(ctx, key, val)


Is this because we're iterating over all the keys of the metadata, but only deleting the last key?

It was my impression that context.WithValue creates a copy rather than mutating the existing instance, so because, here, the copied ctx object goes out of scope at the end of this method, I would not have thought a memory leak was possible.

If it is a long-lived object, how does it behave when multiple calls specify different values for a given key?

chinigo · 2021-03-03T17:07:00Z

pkg/plugin/javascript/dashboard_delete.go

@@ -44,6 +44,16 @@ func (d *DashboardDelete) Call(ctx context.Context, vm *goja.Runtime) func(c goj
 		// This will never error since &key is a pointer to a type.
 		_ = vm.ExportTo(obj, &key)

+		if len(c.Arguments) > 1 {


Javascript doesn't care one way or the other, so either way seems fine to me.

…hive#2099]

xtreme-vikram-yadav · 2021-03-03T21:03:02Z

I'm syncing with @chinigo and @dimitrybesson to address some of the concerns.

Improve test coverage [vmware-archive#2042] Signed-off-by: Vikram Yadav <yvikram@vmware.com> Co-authored-by: Michael Chinigo <mchinigo@vmware.com> Co-authored-by: Dimitry Besson <dbesson@vmware.com>

wwitzel3

This is looking great, I'd like to see a couple minor updates and then I think this is ready.

pkg/plugin/javascript/dashboard_delete.go

pkg/plugin/javascript/dashboard_client.go

pkg/plugin/javascript/dashboard_list.go

- Improve check for missing argument - restrict metadata values to string - Make tests more descriptive Signed-off-by: Vikram Yadav <yvikram@vmware.com>

wwitzel3 · 2021-03-09T15:34:04Z

This is an interesting example of a similar idea, https://github.com/grpc/grpc-go/blob/v1.36.0/metadata/metadata.go#L154 .. I think I would like to see the Key/Value extraction and copying done as a function that we can unit test.

wwitzel3

Sorry @xtreme-vikram-yadav I changed my mind, I would like to see the Key/Value extraction and copy process as a stand-alone function.

I don't think we need to use the go-grpc implementation, but I don't like that pattern of creating a unit testable single purpose function, instead of copying and pasting this in to each client method.

xtreme-vikram-yadav · 2021-03-10T01:17:08Z

I have moved the common extraction logic out. I have kept the unit tests as is though because they are public and should be tested for the function they provide.

Signed-off-by: Vikram Yadav <yvikram@vmware.com>

wwitzel3

LGTM

xtreme-vikram-yadav force-pushed the plugins-context branch from 56df03a to d6ad3d2 Compare March 3, 2021 15:16

xtreme-vikram-yadav commented Mar 3, 2021

View reviewed changes

xtreme-vikram-yadav mentioned this pull request Mar 3, 2021

Add additional metadata to pass context to octant [#2042] vmware-archive/plugin-library-for-octant#8

Merged

Add lookup for additional metadata from JS plugins

9491df7

[vmware-archive#2042] Signed-off-by: Vikram Yadav <yvikram@vmware.com>

xtreme-vikram-yadav force-pushed the plugins-context branch from d6ad3d2 to 9491df7 Compare March 3, 2021 15:42

chinigo reviewed Mar 3, 2021

View reviewed changes

xtreme-vikram-yadav pushed a commit to xtreme-vikram-yadav/octant that referenced this pull request Mar 3, 2021

Avoid memory leak by using new context in a function call [vmware-arc…

203f450

…hive#2099]

xtreme-vikram-yadav force-pushed the plugins-context branch from 203f450 to ec7b3a5 Compare March 3, 2021 18:39

xtreme-vikram-yadav force-pushed the plugins-context branch from ec7b3a5 to 5ac6684 Compare March 4, 2021 00:49

Avoid memory leak by using new context in a function call

f460354

Improve test coverage [vmware-archive#2042] Signed-off-by: Vikram Yadav <yvikram@vmware.com> Co-authored-by: Michael Chinigo <mchinigo@vmware.com> Co-authored-by: Dimitry Besson <dbesson@vmware.com>

xtreme-vikram-yadav force-pushed the plugins-context branch from 5ac6684 to f460354 Compare March 4, 2021 15:51

wwitzel3 suggested changes Mar 5, 2021

View reviewed changes

pkg/plugin/javascript/dashboard_delete.go Outdated Show resolved Hide resolved

pkg/plugin/javascript/dashboard_client.go Outdated Show resolved Hide resolved

pkg/plugin/javascript/dashboard_list.go Outdated Show resolved Hide resolved

Use context.Cancel to clone context

064abf7

- Improve check for missing argument - restrict metadata values to string - Make tests more descriptive Signed-off-by: Vikram Yadav <yvikram@vmware.com>

xtreme-vikram-yadav force-pushed the plugins-context branch from 545e780 to 064abf7 Compare March 5, 2021 19:30

wwitzel3 approved these changes Mar 8, 2021

View reviewed changes

wwitzel3 suggested changes Mar 9, 2021

View reviewed changes

xtreme-vikram-yadav force-pushed the plugins-context branch from 6e8e170 to 88cee0f Compare March 10, 2021 18:33

Extract out key/value extraction into a shared method

0a36383

Signed-off-by: Vikram Yadav <yvikram@vmware.com>

xtreme-vikram-yadav force-pushed the plugins-context branch from 88cee0f to 0a36383 Compare March 10, 2021 22:17

xtreme-vikram-yadav mentioned this pull request Mar 10, 2021

Go plugins context #2136

Closed

wwitzel3 approved these changes Mar 11, 2021

View reviewed changes

wwitzel3 merged commit ec84d18 into vmware-archive:master Mar 11, 2021

xtreme-vikram-yadav mentioned this pull request Mar 11, 2021

Extract store metadata sent by Go plugins #2140

Merged

xtreme-vikram-yadav mentioned this pull request Apr 21, 2021

Approver Request #2355

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lookup for additional metadata from JS plugins #2099

Add lookup for additional metadata from JS plugins #2099

xtreme-vikram-yadav commented Mar 3, 2021

xtreme-vikram-yadav Mar 3, 2021

chinigo Mar 3, 2021

xtreme-vikram-yadav Mar 3, 2021

xtreme-vikram-yadav Mar 3, 2021

xtreme-vikram-yadav Mar 3, 2021

chinigo Mar 3, 2021

xtreme-vikram-yadav Mar 3, 2021

xtreme-vikram-yadav Mar 3, 2021

chinigo left a comment

chinigo Mar 3, 2021

chinigo Mar 3, 2021

xtreme-vikram-yadav commented Mar 3, 2021

wwitzel3 left a comment

wwitzel3 commented Mar 9, 2021

wwitzel3 left a comment

xtreme-vikram-yadav commented Mar 10, 2021

wwitzel3 left a comment

Add lookup for additional metadata from JS plugins #2099

Add lookup for additional metadata from JS plugins #2099

Conversation

xtreme-vikram-yadav commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtreme-vikram-yadav commented Mar 3, 2021

wwitzel3 left a comment

Choose a reason for hiding this comment

wwitzel3 commented Mar 9, 2021

wwitzel3 left a comment

Choose a reason for hiding this comment

xtreme-vikram-yadav commented Mar 10, 2021

wwitzel3 left a comment

Choose a reason for hiding this comment