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

[BUG]: App-analytics - fixed issue for app break on update chart #1147

Conversation

DipraAich
Copy link
Contributor

@DipraAich DipraAich commented Oct 17, 2022

Signed-off-by: Dipra Aich [email protected]

Description

Application Analytics - App does not respond if configuration is added and Update Chart is clicked

Issues Resolved

  • Elimination of error on Update chart click
  • Update of query into query search box on update chart click
  • Restrict reset of configuration of update chart click
  • Fetch and represent proper data on update chart click
  • App analytics internally stores the entire query, i.e including the base query in raw query property,
    but only displays the part excluding the base query in UI.
  • If a query exists in the search panel and use clicks Update chart, the query is updated to the latest.
    It does not concatenate with the previous query.

#1136
#1168

Check List

  • Clicking the Update Chart updates the configuration, chart UI and query properly
  • Commits are signed per the DCO using --signoff

@DipraAich DipraAich requested a review from a team as a code owner October 17, 2022 15:43
@mengweieric
Copy link
Collaborator

@eugenesk24 Need your review and confirmation on their works for this issue: #1136

@eugenesk24
Copy link
Contributor

Waiting for changes addressing my comment before approving

@DipraAich DipraAich force-pushed the bug/app-analytics-update-chart-issues branch from 048a394 to 973e06e Compare October 21, 2022 14:21
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2022

Codecov Report

Merging #1147 (38c06f2) into main (e0a3052) will decrease coverage by 17.68%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##               main    opensearch-project/observability#1147       +/-   ##
=============================================
- Coverage     71.87%   54.18%   -17.69%     
  Complexity      291      291               
=============================================
  Files            42      279      +237     
  Lines          2311     9473     +7162     
  Branches        240     2235     +1995     
=============================================
+ Hits           1661     5133     +3472     
- Misses          509     4169     +3660     
- Partials        141      171       +30     
Flag Coverage Δ
dashboards-observability 48.47% <100.00%> (?)
opensearch-observability 71.87% <ø> (ø)

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

Impacted Files Coverage Δ
...ty/common/query_manager/antlr/ppl_syntax_parser.ts 100.00% <100.00%> (ø)
...ponents/visualizations/charts/helpers/viz_types.ts 44.91% <100.00%> (ø)
...ic/components/visualizations/charts/lines/line.tsx 37.89% <0.00%> (ø)
...fig_panes/config_controls/config_gauge_options.tsx 4.54% <0.00%> (ø)
...omponents/event_analytics/hooks/use_tab_context.ts 100.00% <0.00%> (ø)
...nents/event_analytics/explorer/sidebar/sidebar.tsx 88.46% <0.00%> (ø)
...components/event_analytics/explorer/no_results.tsx 100.00% <0.00%> (ø)
...ublic/components/visualizations/charts/bar/bar.tsx 48.95% <0.00%> (ø)
...ules/visualization_flyout/visualization_flyout.tsx 42.55% <0.00%> (ø)
...izations/config_panel/config_panes/json_editor.tsx 33.33% <0.00%> (ø)
... and 229 more

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

joshuali925
joshuali925 previously approved these changes Oct 21, 2022
if (query.includes(appBaseQuery)) {
if (query.includes('|')) {
// Some scenarios have ' | ' after base query and some have '| '
return query.replace(' | ', '| ').replace(appBaseQuery + '| ', '');
Copy link
Member

Choose a reason for hiding this comment

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

is the if necessary? also this seems flakey. it doesn't handle two spaces before |, or how does it ensure there's only 0 or 1 space before |?

Copy link
Contributor

Choose a reason for hiding this comment

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

All query are pipelines, no matter how they're composed or split.

Can we instead try to use set-wise operations here ;

"...pipeline string...".split('|').map {|token| token.trim() }

And then later

tokens.join("|")

???

Copy link
Contributor

Choose a reason for hiding this comment

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

It is also quite possible that the query can have multiple pipes, source = opensearch_dashboards_sample_logs | where response='403' | sort machine. In this case the replace would only replace the first pipe and therefore the second replace will not match anything and the base query will not be removed from the search bar view.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again... why are we not using set-wise processing on the pipe sections?

Copy link
Member

Choose a reason for hiding this comment

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

@pjfitzgibbons not sure where is the "set", but how do you handle source=index | fields `a|b` ?
i brought up a similar issue before: #81 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eugenesk24 ,

As per the conversations on this PR we have made an implementation which would work irrespective of any amount of spaces present before or after the pipe. But if any field exists with a pipe sign in it like 'a |b' or 'a| b' this will fail. We wanted to confirm if these could possibly be valid fields or not.
Implementation:
const generateViewQuery = (query: string) => {
if (query.includes(appBaseQuery)) {
if (query.includes('|')) {
const newQuery = query
.split('|')
.map((section: string) => section.trim())
.join('|');
return newQuery.replace(appBaseQuery + '|', '');
}
return '';
}
return query;
};

@DipraAich DipraAich force-pushed the bug/app-analytics-update-chart-issues branch from 973e06e to bf68f9a Compare October 28, 2022 13:37
@DipraAich DipraAich force-pushed the bug/app-analytics-update-chart-issues branch from 38c06f2 to 95f5646 Compare November 2, 2022 14:52
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.

6 participants