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

[FEATURE] Initial ScatterChart Panel Plugin #1672

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Conversation

zhuje
Copy link
Contributor

@zhuje zhuje commented Dec 21, 2023

Description

TLDR: Initial ScatterChart Panel Plugin

  • Add Panel Plugin for Scatter Chart
    • Scatter Chart only handles Trace data for now
    • Scatter Chart currently doesn't offer any spec options to format chart (e.g., y-axis label, min value, max value)
  • Add Tempo Query Editor for query editing in UI
    • There is no auto-complete extension in CodeMirror for Tempo

Note

ScatterChart Panel Plugin is based on the work done by Steve Cobb from [perses-embed-example](https://github.com/sjcobb/perses-embed-example).

Backend Changes

Add schemas for scatter plot panels in schemas/panels/scatterplot . In this initial iteration, no spec options are added to this panel.

Frontend Changes

1. Add scatter plot panel for data visualization

  • Files Changes in perses/ui/panels-plugin/src/scatterplot
    • ScatterChart.ts — define a PanelPlugin Chart. The PluginRegistry uses this file to fetch the component dynamically (see the file PanelContent.tsx and the usePlugin() function).
    • ScatterChartPanel.tsx — define a PanelComponent, transforms the Tempo API data into a ‘dataset’ that can be consumed by Apache ECharts. Pass dataset and additional visualization options as props to component.
    • ScatterChartPanel.test.tsx — test if ScatterChartPanel can render correctly
    • Scatterplot.tsx - accept props from Scatterplot and format the Apache EChart with the props.
    • scatter-chart-model.ts - define models for components in ScatterChart.ts

1B. Component TreeView

  • ScatterChart
    • ScatterChartPanel
      • ScatterPlot

2. Changes to add a UI panel editing in a scatter plot with Tempo Data
Note: These changes were based on Time Series Query Editor. In the future, if additional data types are added, we should consider how to create more reusable components (i.e., if we can make a general query editor that is less tightly coupled to a specific query type).

  • File Changes in perses/ui/plugin-system/src/components
    • PanelSpecEditor.tsx — modified to include a switch statement to render the correct query editor based on the dashboard definition spec > query kind (e.g., TimeSeriesQuery, TraceQuery). For example : switch(queryType) >> case ‘TraceQuery’ >> render <TraceQueryEditor>
    • TraceQueryEditor.tsx — holds the logic for handling add/removing queries in trace query editor and passes these as props to TraceQueryInput
    • TraceQueryInput.tsx— Renders collapsing and delete buttons. Contains the PluginEditor component, which uses the Panel’s query type (i.e., TraceQuery) to render the corresponding TraceQuery editing components
      • PluginEditor.tsx
        • PluginKindSelect.tsx — renders a drop-down containing kinds for our query type. For example, <PluginKind queryType={TraceQuery}/> drop-down will render TempoTraceQuery.
        • PluginSpecEditor.tsx — dynamically renders the corresponding OptionsEditorComponent given a query type. For example, <PluginSpecEditor queryType={TraceQuery}/> will fetch the query plugin from the file TempoTraceQuery.tsx and get the OptionsEditorComponent: TempoTraceQueryEditor
  • File Changes in perses/ui/tempo-plugin/src/tempo-trace-query
    • TempoTraceQuery.tsx — define OptionsEditorComponent: TempoTraceQueryEditor in the query plugin
    • TempoTraceQueryEditor.tsx — handles data source change, contains
    • DashboardTraceQueryEditor.tsx — contains and
    • TraceQLEditor.tsx— embed , which is a text input for a query. Currently, there is no autocomplete extension for Tempo.

3. Enable TimeRangeSelector on TraceQuery

  • File changes for /perses/ui/plugin-system/src/runtime
    • trace-queries.tsx - fetch selected time range (e.g. ‘last 1 hour, last 1 day’) and pass it as a prop to the trace fetching function getTraceData() in the get-trace-data.tsx file
  • File changes for perses/ui/tempo-plugin/src/tempo-trace-query
    • get-trace-data.tsx — contains getTraceData(). Modify the query to include a ‘start’ and ‘end’ time. For example {fooQuery}&start={unixStartTime}&end{unixEndTime}. Pass this query to client.getEnrichedTraceQuery(getQuery(), datasourceUrl) to fetch trace data from API.

Screenshots

Figure 1. Scatter Chart
Datapoint size corresponds to the number of spans within a trace.
Red datapoints indicate the trace contains and error.
Perses_Scatterplot_PR_Screenshot_ScatterChart_Dec_20_2023
Figure 2. Trace Query Editor
Perses_Scatterplot_PR_Screenshot_TraceQueryEditor_Dec_20_2023

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
    following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
  • All commits have [DCO signoffs](https://github.com/probot/dco#how-it-works).

UI Changes

    -- Add Panel Plugin for Scatter Chart
    -- Add Tempo Query Editor for UI Editing

Signed-off-by: Jenny Zhu <jenny.a.zhu@gmail.com>
@sjcobb sjcobb self-requested a review December 23, 2023 03:43
Merge from main.

Signed-off-by: Jenny Zhu <jenny.a.zhu@gmail.com>
Signed-off-by: Jenny Zhu <jenny.a.zhu@gmail.com>
startTime: Date;
}

export function getUnixTimeRange(timeRange: AbsoluteTimeRange) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't remember exactly, but we might have a similar util in @perses-dev/core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve removed this in the latest commit. This was a helper function used by greaterThanSevenDays().

// 1. Map x,y coordinates
// 2. Datapoint size corresponds to the number of spans in a trace
// 3. Color datapoint red if the trace contains an error
const seriesTemplate: SeriesOption = {
Copy link
Member

Choose a reason for hiding this comment

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

The dataset lines above are in a useMemo so that's good, but these other lines could benefit from being memozied too. It would break the rules of hooks though so having a separate component like ScatterChartPanelLoaded.tsx would be the way to go (other panel types would benefit from that too)

Copy link
Contributor Author

@zhuje zhuje Jan 13, 2024

Choose a reason for hiding this comment

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

TBH, I’m a novice with hooks and don’t see how it would break the rules. Could you elaborate on the rules that are being broken and the implementation you’d like to see for the separate component ScatterChartPanelLoaded? In the latest push, I attempted to use the useMemo() hook directly in scatterChartPanel to memoize the series formatting, and it didn’t throw any errors…

I am not sure how the rules are being broken because the memoized function is not inside a condition, loop, or event handler, and there are no other hooks within the function. And I moved the error checks that would return the function early, after the hooks.

Copy link
Member

Choose a reason for hiding this comment

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

Oh great! I thought it was after the loading if conditions but I might have misread too, if no errors were thrown or anything then you should be good

return 'red';
}
// Else return default color
const defaultColor = '#56B4E9';
Copy link
Member

Choose a reason for hiding this comment

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

chartsTheme is nice to leverage for these types of colors since it allows embed users to customize their palette, fonts, etc. (ex of defaultColor for thresholds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, agree. Great catch; this is updated in the latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome and you can always add new colors too if what’s there isn’t enough

dataset: options.dataset,
series: options.series,
grid: {
bottom: 40,
Copy link
Member

@sjcobb sjcobb Jan 12, 2024

Choose a reason for hiding this comment

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

Similar comment about possibly leveraging chartsTheme if possible

Edit: NVM, ignore me, I see that grid is pretty custom here due to dataZoom


// Propagate changes to the query's value when the input is blurred to avoid constantly re-running queries in the
// PanelPreview
const handleQueryBlur = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Side note: sometimes this onBlur logic annoys me in our Prometheus editor and I just want an explicit 'Run queries' button 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. This could be generated as a separate issue. Updating the panel to have an explicit 'Run queries' button might also be more intuitive for the user.

Copy link
Member

@sjcobb sjcobb left a comment

Choose a reason for hiding this comment

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

🚀

Signed-off-by: Jenny Zhu <jenny.a.zhu@gmail.com>
@zhuje zhuje requested a review from sjcobb January 13, 2024 18:52
Copy link
Member

@sjcobb sjcobb left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Nexucis Nexucis left a comment

Choose a reason for hiding this comment

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

incredible work @zhuje thank you so much for taking the time to do it !

And thank you @sjcobb for reviewing and @zhuje for your patience

@Nexucis Nexucis added this pull request to the merge queue Jan 17, 2024
Merged via the queue into perses:main with commit aba5521 Jan 17, 2024
18 checks passed
sjcobb added a commit that referenced this pull request Feb 8, 2024
Signed-off-by: Steven Cobb <sjcobb.dev@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 10, 2024
* [BUGFIX] Tooltip positioning broken in Firefox

Signed-off-by: Steven Cobb <sjcobb.dev@gmail.com>

* fix lint from Scatterplot #1672, remove totalSeries

Signed-off-by: Steven Cobb <sjcobb.dev@gmail.com>

---------

Signed-off-by: Steven Cobb <sjcobb.dev@gmail.com>
@sjcobb sjcobb mentioned this pull request Jun 16, 2024
6 tasks
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

3 participants