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

[Vis Colors] Update color mapper to prioritize unique colors per vis #4890

Merged

Conversation

joshuarrrr
Copy link
Member

@joshuarrrr joshuarrrr commented Aug 31, 2023

Description

Instead of trying to generate unique colors for an entire dashboard.

Visualizations that use the color mapper (VisLib, VisBuilder, TSVB) will now have the following behavior:

  1. Every visualization will preferentially use the colors from the OUI qualitative palette. Only if there are more than 10 colors needed for a single visualization, additional colors will be generated via rotations of that palette. (new feature)
  2. Mapped colors on dashboards are no longer subject to race conditions, where multiple loads of the same dashboard would result in different mapped colors. That means dashboard colors are stable and there's no need to manually specify colors just to achieve consistency. (new feature)
  3. Series labels that appear in multiple visualization in the same dashboard (e.g. status code "404") will have the same color, even if they're ordered differently. (existing feature maintained)
  4. Within an individual visualization, every data series will get a unique color with no repeats (existing feature maintained)
  5. Custom color configurations specified via the visualization:colorMapping advanced setting will be respected. Additionally, if one or more series in a visualization have custom color configurations that use OUI qualitative palette colors, those will be skipped when mapping non-customized colors to avoid duplicates. (new feature)
  6. If users manually set colors via the legend (either in visualization or dashboard), it will not affect the color mapping (those are treated as overrides to mapped colors). (existing feature maintained)

Issues Resolved

Fixes #4697

Screenshot

Sample dashboards (next light) after

Notice the consistency across dashboards and data sets:
Screenshot 2023-08-31 at 18-35-14 Flights Global Flight Dashboard - OpenSearch Dashboards
Screenshot 2023-08-31 at 18-34-24 Logs Web Traffic - OpenSearch Dashboards
Screenshot 2023-08-31 at 18-33-43 eCommerce Revenue Dashboard - OpenSearch Dashboards

Sample dashboards (next light) before

Screenshot 2023-08-14 at 15-06-50 eCommerce Revenue Dashboard - OpenSearch Dashboards
Screenshot 2023-08-14 at 15-07-28 Logs Web Traffic - OpenSearch Dashboards
Screenshot 2023-08-14 at 15-08-17 Flights Global Flight Dashboard - OpenSearch Dashboards

Visual Consistency Dashboard (all themes) after

Screenshot 2023-08-31 at 18-26-15 Visual Consistency Dashboard Copy - OpenSearch Dashboards

Screenshot 2023-08-31 at 18-30-22 Visual Consistency Dashboard Copy - OpenSearch Dashboards

Screenshot 2023-08-31 at 18-32-00 Visual Consistency Dashboard Copy - OpenSearch Dashboards

Screenshot 2023-08-31 at 18-32-00 Visual Consistency Dashboard Copy - OpenSearch Dashboards

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Instead of trying to generate unique colors for an entire dashboard

Signed-off-by: Josh Romero <rmerqg@amazon.com>
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #4890 (6c5b497) into main (ce2d79e) will decrease coverage by 0.06%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4890      +/-   ##
==========================================
- Coverage   66.39%   66.34%   -0.06%     
==========================================
  Files        3396     3397       +1     
  Lines       64801    64805       +4     
  Branches    10359    10360       +1     
==========================================
- Hits        43025    42994      -31     
- Misses      19217    19222       +5     
- Partials     2559     2589      +30     
Flag Coverage Δ
Linux_1 ?
Linux_2 ?
Linux_3 ?
Linux_4 ?
Windows_1 34.86% <33.33%> (-0.01%) ⬇️
Windows_2 55.12% <ø> (ø)
Windows_3 44.24% <100.00%> (+<0.01%) ⬆️
Windows_4 34.92% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...rc/plugins/charts/public/services/colors/colors.ts 100.00% <ø> (ø)
...ins/charts/public/services/colors/mapped_colors.ts 96.42% <100.00%> (+3.83%) ⬆️

... and 23 files with indirect coverage changes

Signed-off-by: Josh Romero <rmerqg@amazon.com>
let newColors = _.difference(colorPalette, allColors);
})
.filter((color) => !alreadyUsedColors.includes(color.toLowerCase()))
.slice(0, keysToMap.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check that after filtering, we have enough colors? as much as keysToMap.length? or we don't need to worry about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@AMoo-Miki I made this change as more of a surgical minimal change rather than a larger refactor, which this utility could probably use.

My intent is that it will always be the correct length, because we're using the total keys.length in L110 to generate the colors, and then we're only filtering out alreadyUsedColors. The intent of the L88-106 block is to push every key in keys to either alreadyUsedColors or keysToMap. But it could certainly be written cleaner to guarantee that's the case.

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Thank you!

@joshuarrrr joshuarrrr merged commit abcef34 into opensearch-project:main Sep 1, 2023
58 checks passed
@joshuarrrr joshuarrrr deleted the feat/rotate-mapped-colors branch September 1, 2023 20:14
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 1, 2023
…4890)

* [Vis Colors] Update color mapper to prioritize unique colors per vis

Instead of trying to generate unique colors for an entire dashboard

Signed-off-by: Josh Romero <rmerqg@amazon.com>

* Update CHANGELOG.md

Signed-off-by: Josh Romero <rmerqg@amazon.com>

---------

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit abcef34)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
joshuarrrr pushed a commit that referenced this pull request Sep 1, 2023
…4890) (#4898)

* [Vis Colors] Update color mapper to prioritize unique colors per vis

Instead of trying to generate unique colors for an entire dashboard

Signed-off-by: Josh Romero <rmerqg@amazon.com>

* Update CHANGELOG.md

Signed-off-by: Josh Romero <rmerqg@amazon.com>

---------

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit abcef34)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

[Vis colors] Update color mapping to rotate categorical colors instead of using tints and shades
3 participants