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

Interval annotations on charts broken in master #30347

Closed
3 tasks done
martyngigg opened this issue Sep 20, 2024 · 3 comments · Fixed by #30348
Closed
3 tasks done

Interval annotations on charts broken in master #30347

martyngigg opened this issue Sep 20, 2024 · 3 comments · Fixed by #30348
Labels
explore:annotation Related to annotations in Explore explore Namespace | Anything related to Explore viz:charts Namespace | Anything related to viz types

Comments

@martyngigg
Copy link
Contributor

Bug description

Annotation layers of type "Interval" no longer seem to appear on line charts in master. They are fine in 4.02-dev and 4.1.0rc2-dev but not on master.

It does not matter if the source is a 'Superset annotation' or a 'Table'.

How to reproduce the bug

This uses the examples datasets.

  1. Spin up the latest development image: TAG=994de1f-dev docker compose -f ./docker-compose-image-tag.yml up -d
  2. Go to 'Settings -> Annotation Layers'.
  • Click + Annotation layer button
  • Give the layer a name
  • Click Add
  1. Click on the name of the new annotation layer
  • Click + Annotation button
  • Give the annotation a name
  • Set the start date to 2000-01-01 00:00:00 and end date to 2004-01-01 00:00:00
  • Click Add
  1. Click Charts in the navigation bar
  • Click + Chart
  • Select the birth_names dataset
  • Select the Line Chart type
  • Click Create new chart
  1. Configure the chart
  • Drag the ds column to X-axis
  • Drag the num column to Metrics, choose the Sum aggregate and click Save
  • Scroll down in the configure panel and click Annotations and Layers followed by + Add annotation layer
  • Give the layer a name
  • Check the Show label box
  • Change the Annotation layer type to Interval
  • Change the Annotation source to Superset annotation
  • Change the Annotation layer to the name given to the newly created layer in step 2
  • Click OK
  • Click Update chart

I cannot see any errors in the superset logs. I've attached a screenshot of an error in the Firefox console.

Screenshots/recordings

Superset-Missing-Annotations

Superset version

master / latest-dev

Python version

3.10

Node version

18 or greater

Browser

Firefox

Additional context

No response

Checklist

  • I have searched Superset docs and Slack and didn't find a solution to my problem.
  • I have searched the GitHub issue tracker and didn't find a similar bug report.
  • I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.
@dosubot dosubot bot added explore Namespace | Anything related to Explore explore:annotation Related to annotations in Explore viz:charts Namespace | Anything related to viz types labels Sep 20, 2024
Copy link

dosubot bot commented Sep 20, 2024

@martyngigg Hello! I'm here to help you with any bugs, questions, or contributions. Let's get this sorted out together!

I found a similar closed issue: Line Chart annotations don't load on Time-Series Line Charts. The issue was fixed on the master branch and will be included in the upcoming 2.1.0 and/or 3.0 releases. If needed, the fix can be isolated to a single PR and applied to version 2.0.2 by making a request on the specified thread [1].

The interval annotations on line charts are not appearing in the master version of Apache Superset because the code responsible for updating the annotation positions on brush events is missing in the master version. This code is present in the 4.02-dev and 4.1.0rc2-dev versions. The following snippet is responsible for updating the interval annotations on brush events:

// update annotation positions on brush event
if (chart.focus) {
  chart.focus.dispatch.on('onBrush.interval-annotation', () => {
    annotations
      .selectAll('rect')
      .data(records)
      .attr({
        x: d => xScale(new Date(d[e.timeColumn])),
        width: d => {
          const x1 = xScale(new Date(d[e.timeColumn]));
          const x2 = xScale(new Date(d[e.intervalEndColumn]));
          return Math.max(Math.abs(x2 - x1), 1);
        },
      });
  });
}

Additionally, the transformProps function in the Timeseries plugin for Echarts in Apache Superset is responsible for transforming the chart properties, including annotations. The test cases for transformProps show that interval annotations are expected to be included in the transformed properties. If the interval annotations are not appearing, it could be due to a missing or incorrect implementation in the transformProps function in the master version [2][3][4].

To continue talking to Dosu, mention @dosu.

@kgabryje
Copy link
Member

@martyngigg #30348 should have fixed the bug. Closing this issue, feel free to reopen if the bug persists

@martyngigg
Copy link
Contributor Author

@kgabryje Yep looks good in my example now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explore:annotation Related to annotations in Explore explore Namespace | Anything related to Explore viz:charts Namespace | Anything related to viz types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants