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 Builder] Bug fixes for datasource picker and auto time interval #2632

Merged
merged 6 commits into from
Oct 26, 2022

Conversation

ashwin-pc
Copy link
Member

@ashwin-pc ashwin-pc commented Oct 20, 2022

Description

  • Fixes Date histogram auto bounds says per 0 millisecond
  • Fixes Histogram updating bounds when date range updates
  • Fixes Switching index pattern while editing agg breaks experience

Issues Resolved

#2531 #2310

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #2632 (3e68839) into main (76d30ec) will decrease coverage by 0.01%.
The diff coverage is 13.33%.

@@            Coverage Diff             @@
##             main    #2632      +/-   ##
==========================================
- Coverage   66.78%   66.76%   -0.02%     
==========================================
  Files        3207     3208       +1     
  Lines       61175    61193      +18     
  Branches     9329     9331       +2     
==========================================
+ Hits        40855    40858       +3     
- Misses      18084    18100      +16     
+ Partials     2236     2235       -1     
Impacted Files Coverage Δ
...pplication/components/data_tab/use/use_dropbox.tsx 5.97% <0.00%> (+0.33%) ⬆️
...s_builder/public/application/components/option.tsx 0.00% <ø> (ø)
...uilder/public/application/components/workspace.tsx 4.00% <0.00%> (ø)
...public/application/utils/state_management/store.ts 15.00% <ø> (ø)
...tion/utils/state_management/visualization_slice.ts 16.66% <0.00%> (-0.48%) ⬇️
...ilder/public/embeddable/vis_builder_embeddable.tsx 0.98% <0.00%> (-0.01%) ⬇️
...ublic/services/type_service/visualization_type.tsx 100.00% <ø> (ø)
...lder/public/visualizations/metric/to_expression.ts 6.12% <ø> (ø)
.../public/visualizations/vislib/common/create_vis.ts 11.11% <0.00%> (-5.56%) ⬇️
...s_builder/public/application/utils/use/use_aggs.ts 5.88% <5.88%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ananzh ananzh added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 20, 2022
joshuarrrr
joshuarrrr previously approved these changes Oct 20, 2022
@@ -17,5 +19,14 @@ export const createVis = async (
vis.data.searchSource = await getSearchService().searchSource.create();
vis.data.searchSource.setField('index', indexPattern);

const responseAggs = vis.data.aggs.getResponseAggs().filter((agg) => agg.enabled);
responseAggs.forEach((agg) => {
if (isDateHistogramBucketAggConfig(agg)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be conditional? Or what would be the harm of setting the timerange param for all aggs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I know the date_histogram bucket agg wont construct the label correctly without this value, but i dont see any reason why it would be a problem for other aggs too. Also this makes me want to spend a lil more time on this change to understand how the agg timeranges work. will update it in a revision

joshuarrrr
joshuarrrr previously approved these changes Oct 21, 2022
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

I'll admit that I didn't fully follow all the implications of these changes, but this looks much cleaner to me now.

@ashwin-pc
Copy link
Member Author

I'll admit that I didn't fully follow all the implications of these changes, but this looks much cleaner to me now.

Thanks for calling that out. Ideally I shouldn't be clubbing more than a single change in a PR but had to since these were all small changes that would have increased churn a lot of they were separate. That being said let me add comments to each change for future visibility.

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
@@ -63,8 +63,7 @@ export class SimpleSavedObject<T = unknown> {
error,
references,
migrationVersion,
// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is simply to remove the eslint exception

@@ -50,12 +50,13 @@ export const Workspace: FC = ({ children }) => {
setExpression(undefined);
return;
}
const exp = await toExpression(rootState);

const exp = await toExpression(rootState, searchContext);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now passing search context too since certain visualizations need the search context data to correctly configure the expression

* @param state visualization state
* @returns [Validity, 'Message']
*/
export const validateSchemaState = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Simplified this since we can only reliably validate on he visualization state.

dirty = true;
}

if (this.handler && dirty) {
this.updateHandler();
if (dirty) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We want to recalculate the expression if the context changes too.

Copy link
Member

@abbyhu2000 abbyhu2000 left a comment

Choose a reason for hiding this comment

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

Thanks for the comments!

@ashwin-pc ashwin-pc merged commit 7a41d94 into opensearch-project:main Oct 26, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2632-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7a41d9475451be7ae3125590645b0b9a73e8dbb3
# Push it to GitHub
git push --set-upstream origin backport/backport-2632-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2632-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport-2632-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7a41d9475451be7ae3125590645b0b9a73e8dbb3
# Push it to GitHub
git push --set-upstream origin backport-2632-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport-2632-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.4 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.4 2.4
# Navigate to the new working tree
pushd ../.worktrees/backport-2.4
# Create a new branch
git switch --create backport-2632-to-2.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7a41d9475451be7ae3125590645b0b9a73e8dbb3
# Push it to GitHub
git push --set-upstream origin backport-2632-to-2.4
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.4

Then, create a pull request where the base branch is 2.4 and the compare/head branch is backport-2632-to-2.4.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 3, 2022
…2632)

* Fixes auto time interval

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes change datasource while editing agg

Signed-off-by: Ashwin P Chandran <[email protected]>

* Misc fixes

Signed-off-by: Ashwin P Chandran <[email protected]>

* Updates Changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

* Correctly sets timerange

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes rebase

Signed-off-by: Ashwin P Chandran <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 7a41d94)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AMoo-Miki pushed a commit that referenced this pull request Nov 3, 2022
…2632) (#2765)

* Fixes auto time interval

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes change datasource while editing agg

Signed-off-by: Ashwin P Chandran <[email protected]>

* Misc fixes

Signed-off-by: Ashwin P Chandran <[email protected]>

* Updates Changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

* Correctly sets timerange

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes rebase

Signed-off-by: Ashwin P Chandran <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 7a41d94)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.4 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.4 2.4
# Navigate to the new working tree
pushd ../.worktrees/backport-2.4
# Create a new branch
git switch --create backport-2632-to-2.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7a41d9475451be7ae3125590645b0b9a73e8dbb3
# Push it to GitHub
git push --set-upstream origin backport-2632-to-2.4
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.4

Then, create a pull request where the base branch is 2.4 and the compare/head branch is backport-2632-to-2.4.

ananzh pushed a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Nov 3, 2022
…pensearch-project#2632) (opensearch-project#2765)

* Fixes auto time interval

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes change datasource while editing agg

Signed-off-by: Ashwin P Chandran <[email protected]>

* Misc fixes

Signed-off-by: Ashwin P Chandran <[email protected]>

* Updates Changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

* Correctly sets timerange

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes rebase

Signed-off-by: Ashwin P Chandran <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 7a41d94)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
…pensearch-project#2632)

* Fixes auto time interval

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes change datasource while editing agg

Signed-off-by: Ashwin P Chandran <[email protected]>

* Misc fixes

Signed-off-by: Ashwin P Chandran <[email protected]>

* Updates Changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

* Correctly sets timerange

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes rebase

Signed-off-by: Ashwin P Chandran <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Arpit-Bandejiya pushed a commit to Arpit-Bandejiya/OpenSearch-Dashboards that referenced this pull request Jan 13, 2023
…pensearch-project#2632)

* Fixes auto time interval

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes change datasource while editing agg

Signed-off-by: Ashwin P Chandran <[email protected]>

* Misc fixes

Signed-off-by: Ashwin P Chandran <[email protected]>

* Updates Changelog

Signed-off-by: Ashwin P Chandran <[email protected]>

* Correctly sets timerange

Signed-off-by: Ashwin P Chandran <[email protected]>

* Fixes rebase

Signed-off-by: Ashwin P Chandran <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Arpit Bandejiya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x v2.4.0 'Issues and PRs related to version v2.4.0' vis builder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants