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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graph NG: exemplars support WIP #28071

Merged
merged 8 commits into from Oct 9, 2020
Merged

Graph NG: exemplars support WIP #28071

merged 8 commits into from Oct 9, 2020

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented Oct 7, 2020

This PR brings some refactorings in order to support exemplars in the same way as annotations.

Main highlights of the changes proposed:

  1. EventsCanvas - enables rendering events on top of the plot area. Currently accepts a generic type T extends AnnotationEvent that we will probably need to adjust to something shared between annotations and exemplars. Uses simple primitive XYCavas under the hood for the Markers to be rendered in correct positions
  2. AnnotationsPlugin and new ExemplarsPlugin now use EventCanvas and corresponding markers under the hood.
  3. PlotContextType was refactored to use getters instead of properties. Also, instead of relying on weird conditions in the plugins (i.e. plotCtx.u && plotCtx.u.series && plotCtx.canvas) the context now exposes isPlotReady property tat indicates it's safe to use all the getters.

TODO:

  • ExemplarsPlugin now uses annotations as input and mocks the y axis value. We need to update the y axis value to be retrieved using uPlot's valueToPos API. For this to work we need to identify which scale are the exemplars using. @zoltanbedi currently the scales in the graph are created using Field's unit configuration. If there is no unit configured then the default scale is called '_fixed'. I assume for the exemplars we won't have a unit? What's the thinking about this?
  • Annotation & exemplar markers now use the same tooltip, which is pretty much a copy of each other. We need to implement a more generic component to handle rendering Markers with a tooltip

@dprokop dprokop added this to the 7.3 milestone Oct 7, 2020
@dprokop dprokop self-assigned this Oct 7, 2020
@dprokop dprokop added this to In Review in Frontend Platform Backlog via automation Oct 7, 2020
Comment on lines +11 to +12
// An abstraction over a component rendered within a chart canvas.
// Marker is rendered with DOM coords of the chart bounding box.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
// An abstraction over a component rendered within a chart canvas.
// Marker is rendered with DOM coords of the chart bounding box.
/**
* An abstraction over a component rendered within a chart canvas.
* Marker is rendered with DOM coords of the chart bounding box.
*/

@dprokop dprokop mentioned this pull request Oct 7, 2020
55 tasks
@dprokop dprokop linked an issue Oct 7, 2020 that may be closed by this pull request
55 tasks
Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

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

I really like this! Left some minor nits for consideration.

packages/grafana-ui/src/components/uPlot/Plot.tsx Outdated Show resolved Hide resolved
packages/grafana-ui/src/components/uPlot/Plot.tsx Outdated Show resolved Hide resolved
canvasRef: any;
data: DataFrame;
}

export const PlotContext = React.createContext<PlotContextType | null>(null);
export const PlotContext = React.createContext<PlotContextType>({} as PlotContextType);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think using null is clearer here, but no biggie

`
)}
>
<div ref={markerRef} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave} className={cx(styles.markerWrapper)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't think you need cx in this case

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Awesome job!

Think there are some follow-up issues we need to look at.

  • Not sure if we should continue using AnnotationEvent, think @ryantxu have plans to deprecate that and use DataFrame directly instead. For example, AnnotationEvent does not have data links that we need for the exemplars. but the field has data links.
  • Observable & useEffects only for simple data model transforms feels inefficient (both for this here and in the main GraphPanel for getting aligned data). Need sync util functions for merge & join transforms.

@dprokop
Copy link
Member Author

dprokop commented Oct 7, 2020

Not sure if we should continue using AnnotationEvent, think @ryantxu have plans to deprecate that and use DataFrame directly instead. For example, AnnotationEvent does not have data links that we need for the exemplars. but the field has data links.

Yup, aware of that. Using the AnnotationEvent here is just to keep the ball rolling :)

@dprokop
Copy link
Member Author

dprokop commented Oct 8, 2020

@torkelo @ryantxu removed the usage of the AnnotationEvent in favor of DataFrame (and DataFrameView for fields inspection) in Annotations & Exemplars plugins. FYI @zoltanbedi

@dprokop dprokop merged commit a3d1d9a into master Oct 9, 2020
Frontend Platform Backlog automation moved this from In Review to Done Oct 9, 2020
@dprokop dprokop deleted the graph-ng-exemplars branch October 9, 2020 09:38
ryantxu pushed a commit that referenced this pull request Nov 18, 2020
* Use annotations data observable

* WIP exemplars

* Refactor usePlotContext to use getters instead of properties

* Use DataFrame in EventsCanvas instead of custom type

* Minor tweaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph NG
3 participants