Utils: Spread objects in cloneSceneObjectState #967

kaydelaney · 2024-11-14T13:05:39Z

Fixes an issue where if a panel was duplicated, both the original panel and duplicated panel's queries state (in SceneQueryRunner) referred to the same object in memory, meaning when the duplicate query was changed, so was the original.

Thread: https://raintank-corp.slack.com/archives/C03KVDHTWAH/p1730897481961219

📦 Published PR as canary version: 6.1.2--canary.967.13520771706.0

✨ Test out this PR locally via:

npm install @grafana/[email protected]
npm install @grafana/[email protected]
# or 
yarn add @grafana/[email protected]
yarn add @grafana/[email protected]

torkelo · 2024-11-14T13:08:09Z

where do we mutate the queries object?

kaydelaney · 2024-11-14T13:14:16Z

@torkelo It might be something strange some datasources do - could only reproduce with graphite/Mock datasource but not prometheus for example. I'll check.

kaydelaney · 2024-11-14T13:55:25Z

Okay so looking into a bit more, at least in the case of Graphite, I think what's happening is it's taking the target that's passed to it and ultimately manipulating that when the query is modified via the graphite query editor.

https://github.com/grafana/grafana/blob/6abe99efd64b8867112d9d9c74971d96840ce32d/public/app/plugins/datasource/graphite/state/context.tsx#L91

Changing the above to target: { ...query } also seems to fix the issue, but other data sources also seem to make the same mistake, so it might be worthwhile "fixing" in scenes just the same.

packages/scenes/src/core/sceneGraph/utils.ts

leeoniya · 2025-01-02T02:45:42Z

this bug was also reported for Google Cloud Monitoring datasource: grafana/grafana#97223 (comment)

axelavargas

Hey @kaydelaney 👋🏾 , as other mentioned, can we fix this issue in the problematic data sources that are doing the mutation? There might be more side effects on implementing this approach

oscarkilhed · 2025-01-14T11:30:08Z

Is there really more side effects to implementing this approach? If I were to clone a panel I wouldn't expect properties on it to be referring to the same property as the old panel, I think this has a great potential to cause more bugs in the future.

That said, the mutation in the graphite DS is also not great and should probably be fixed too.

mdvictor · 2025-01-21T10:08:28Z

packages/scenes/src/core/sceneGraph/utils.ts

@@ -45,6 +45,8 @@ export function cloneSceneObjectState<TState extends SceneObjectState>(
      for (const child of propValue) {
        if (child instanceof SceneObjectBase) {
          newArray.push(child.clone());
+        } else if (typeof child === 'object' && !Array.isArray(child)) {
+          newArray.push({ ...child });


An issue just popped up around transformations that is kind of related to this PR. Basically, duplicating a panel with some transformation and then modifying the transformation options within the second panel would also modify the transformation options in the initial panel.

This happens because of the spread operator won't cover objects that contain deeper nested arrays, which is the case for transformations where you'd have an object of the form:

{ id: "convertFieldType", options: { conversions: [{ ...some objects in array }] } }

Modifying this line to use lodash cloneDeep instead of spread op fixes the issue with transformations, so maybe we can use that?

Thanks for the info! I worry that using cloneDeep might introduce some performance issues considering how often scene objects are cloned, but I'd need to benchmark it to be sure. I'll look into it :)

Indeed, I share your worry, thought about it myself, not sure how to deal with it tho. Some benchmarking would be great!

@mdvictor did you locate where the transformations editors mutate the state?

I didn't have time to further look into the transformation one yet. Basically any modification in a transformation editor in a duplicated panel will mutate the options in the original one because it's the same object reference.

kaydelaney · 2025-01-24T14:11:14Z

Looked into the performance of cloning for three different solutions: Spreading object properties (which has its own limitations as mentioned by @mdvictor re: transformations), structuredClone, and lodash's cloneDeep function.

I evaluated the performance on a 2022 M2 macbook air by taking a typical dashboard scene then cloning it ~1000 times, measuring the times taken to clone the scene, and calculating min, max, median, and mean times to clone for each algorithm.
I did this in Chrome, Firefox, and Safari, which you can see the results for below. Each metric has two values as I ran the test twice.

Firefox

Spread

Min: [0ms, 0ms]
Max: [8ms, 7ms]
Median: [0ms, 0ms]
Mean: [0.267ms, 0.188ms]

structuredClone

Min: [0ms, 0ms],
Max: [9ms, 6ms],
Median: [0ms, 0ms]
Mean: [0.335ms, 0.397ms]

cloneDeep

Min: [0ms, 0ms],
Max: [9ms, 7ms],
Median: [0ms, 0ms],
Mean: [0.362ms, 0.362ms]

Chrome

Spread

Min: [0ms, 0ms]
Max: [5.5ms, 6.1ms]
Median: [0.1ms, 0.1ms]
Mean: [0.175ms, 0.170ms]

structuredClone

Min: [0.19ms, 0.19ms]
Max: [7.89ms, 5.4ms]
Median: [0.4ms, 0.3ms]
Mean: [0.44ms, 0.28ms]

cloneDeep

Min: [0.1ms, 0.1ms]
Max: [6.1ms, 9.8ms]
Median: [0.2ms, 0.2ms]
Mean: [0.285ms, 0.37ms]

Safari

Spread

Min: [0ms, 0ms],
Max: [7ms, ~1ms],
Median: [0ms, 0ms],
Mean: [0.129ms, 0.122ms]

structuredClone

Min: [0ms, 0ms]
Max: [3ms, 3ms]
Median: [0ms, 0ms]
Mean: [0.33ms, 0.3259ms]

cloneDeep

Min: [0ms, 0ms]
Max: [2ms, 1ms]
Median: [0ms, 0ms]
Mean: [0.307ms, 0.316ms]

So, performance takes a small hit if we use structuredClone or cloneDeep, but I think it's still acceptable, and shouldn't negatively affect the UX in any perceivable way.

As for whether we should go with structuredClone or cloneDeep, cloneDeep seems to have slightly better performance for the median case, so I'm leaning towards that.

Changes have been made

leeoniya · 2025-02-24T14:27:23Z

As for whether we should go with structuredClone or cloneDeep, cloneDeep seems to have slightly better performance for the median case, so I'm leaning towards that.

personally, i would opt for native methods if the perf difference is not large. less dependencies.

kaydelaney · 2025-02-24T14:28:50Z

@leeoniya In general I'd agree, but lodash in particular is so ingrained in grafana (and its plugin ecosystem) that I'm not sure it's a big concern

leeoniya · 2025-02-24T14:32:10Z

definitely not a big concern

axelavargas

Hey @kaydelaney , code wise looks good, however, when testing locally found some issues with a dashboard, I got "loading plugin panel" all over the place

I shared more context here

axelavargas

@kaydelaney after extensive testing using this Scene version in a cloud Grafana instance (latest main 11.6.x) and local instance, I did not find anything else 🥳

I checked:

Editing panels that were duplicated works as expected ✅
DataTrails - History steps works as expected ✅
Editing Dashboard Json from editing ✅

TestEphemeralInstance.mp4

scenes-repo-bot-access-token · 2025-02-26T10:16:38Z

🚀 PR was released in v6.1.2 🚀

Utils: Spread objects in cloneSceneObjectState

124f572

kaydelaney added type/bug Something isn't working patch Increment the patch version when merged release Create a release when this pr is merged labels Nov 14, 2024

kaydelaney requested review from torkelo, axelavargas and dprokop November 14, 2024 13:05

kaydelaney self-assigned this Nov 14, 2024

dprokop previously requested changes Nov 19, 2024

View reviewed changes

packages/scenes/src/core/sceneGraph/utils.ts Outdated Show resolved Hide resolved

leeoniya mentioned this pull request Jan 2, 2025

Panel Edit: Edit Panel-B causes Panel-A to get changed also grafana/grafana#98351

Open

axelavargas reviewed Jan 8, 2025

View reviewed changes

kaydelaney mentioned this pull request Jan 16, 2025

Graphite: Fix issue where duplicated panels share same query object grafana/grafana#99077

Merged

grafana-delivery-bot bot mentioned this pull request Jan 16, 2025

[v11.4.x] Graphite: Fix issue where duplicated panels share same query object grafana/grafana#99100

Merged

Don't spread arrays

72b5bea

kaydelaney requested a review from dprokop January 17, 2025 12:23

mdvictor reviewed Jan 21, 2025

View reviewed changes

kaydelaney added 2 commits January 24, 2025 15:35

Use cloneDeep for objects

410e936

fix faulty logic

a7996d6

kaydelaney requested a review from mdvictor January 24, 2025 16:12

run prettier

7f17677

kaydelaney requested a review from axelavargas February 4, 2025 13:25

kaydelaney added 2 commits February 24, 2025 10:53

Merge remote-tracking branch 'origin/main' into clone-fix

215e508

Fix failing test

500941b

axelavargas reviewed Feb 25, 2025

View reviewed changes

Small change so we don't try to apply cloneDeep to functions/PoD

aec403d

kaydelaney requested a review from axelavargas February 25, 2025 13:56

axelavargas mentioned this pull request Feb 26, 2025

Test: CloneDeep Scenes PR grafana/grafana#101340

Closed

axelavargas approved these changes Feb 26, 2025

View reviewed changes

kaydelaney merged commit 57737ad into main Feb 26, 2025
5 checks passed

kaydelaney deleted the clone-fix branch February 26, 2025 10:08

scenes-repo-bot-access-token bot added the released label Feb 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utils: Spread objects in cloneSceneObjectState #967

Utils: Spread objects in cloneSceneObjectState #967

kaydelaney commented Nov 14, 2024 •

edited by scenes-repo-bot-access-token bot

Loading

torkelo commented Nov 14, 2024

kaydelaney commented Nov 14, 2024

kaydelaney commented Nov 14, 2024

leeoniya commented Jan 2, 2025

axelavargas left a comment

oscarkilhed commented Jan 14, 2025

mdvictor Jan 21, 2025

kaydelaney Jan 21, 2025

mdvictor Jan 21, 2025 •

edited

Loading

torkelo Jan 23, 2025

mdvictor Jan 23, 2025

kaydelaney commented Jan 24, 2025 •

edited

Loading

leeoniya commented Feb 24, 2025

kaydelaney commented Feb 24, 2025

leeoniya commented Feb 24, 2025

axelavargas left a comment

axelavargas left a comment

scenes-repo-bot-access-token bot commented Feb 26, 2025

Utils: Spread objects in cloneSceneObjectState #967

Utils: Spread objects in cloneSceneObjectState #967

Conversation

kaydelaney commented Nov 14, 2024 • edited by scenes-repo-bot-access-token bot Loading

torkelo commented Nov 14, 2024

kaydelaney commented Nov 14, 2024

kaydelaney commented Nov 14, 2024

leeoniya commented Jan 2, 2025

axelavargas left a comment

Choose a reason for hiding this comment

oscarkilhed commented Jan 14, 2025

mdvictor Jan 21, 2025

Choose a reason for hiding this comment

kaydelaney Jan 21, 2025

Choose a reason for hiding this comment

mdvictor Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

torkelo Jan 23, 2025

Choose a reason for hiding this comment

mdvictor Jan 23, 2025

Choose a reason for hiding this comment

kaydelaney commented Jan 24, 2025 • edited Loading

Firefox

Spread

structuredClone

cloneDeep

Chrome

Spread

structuredClone

cloneDeep

Safari

Spread

structuredClone

cloneDeep

leeoniya commented Feb 24, 2025

kaydelaney commented Feb 24, 2025

leeoniya commented Feb 24, 2025

axelavargas left a comment

Choose a reason for hiding this comment

axelavargas left a comment

Choose a reason for hiding this comment

scenes-repo-bot-access-token bot commented Feb 26, 2025

kaydelaney commented Nov 14, 2024 •

edited by scenes-repo-bot-access-token bot

Loading

mdvictor Jan 21, 2025 •

edited

Loading

kaydelaney commented Jan 24, 2025 •

edited

Loading