SceneQueryRunner: Meaningful cloning #652
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dprokop
added
type/bug
dprokop
force-pushed
the
sqr-meaningful-clone
branch
from
3987b4d
to
e876d63
Compare
torkelo
approved these changes
Mar 20, 2024
|
🚀 PR was released in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug I have discovered in core Grafana: grafana/grafana#84818
The problem is that in the PanelEditor in core Grafana we are using the cloned version of the data provider. When it's activated, it uses data layers results (via ReplaySubject in the layer) causing the
onLayersReceivedbeing called. But, this function depends on SQR's private_resultAnnotationspropert, that is not cloned given it's a instance property, not part of the object state. Hence we are losing the query-provided annotations.However, it's actually reflecting a state of an object as it holds annotation results that might have been part of a query response. So, it is technically SQR's state.
There are alternatives to this approach:
_resultAnnotations/_layerAnnotationsinstance properties into SQR's state. This would be a bigger refactor. Also leaks the implementation details to a public state. This is next scenario when SceneObject private state would beuseful :)_resultAnnotations/_layerAnnotationspublic properties or provide setters to allow setting externally. Again, leaking internals. Also would delegate the complexity of remembering about setting those when cloning to the consumer. Sounds like a bad idea.So I consider the proposed approach quite elegant, as it does not yield an API change while providing the required functionality.