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

Bugfix/metric-loading-loop #1309

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Bugfix/metric-loading-loop #1309

merged 2 commits into from
Dec 19, 2023

Conversation

pjfitzgibbons
Copy link
Contributor

@pjfitzgibbons pjfitzgibbons commented Dec 19, 2023

Description

Found several bugs in Metric workflow during validation of recent PR

  • Visualization action-menu correction for OSD Dashboards context and Menu Explorer context
  • Visualization of Promql metric navigate to Metric Explorer on "Edit"
  • Metric Explorer correct loading of metric by URL Id

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…isualizationContainer when Promql metric

Signed-off-by: Peter Fitzgibbons <[email protected]>
* Visualization action-menu correction for OSD Dashboards context and Menu Explorer context
* Visualization of Promql metric navigate to Metric Explorer on "Edit"
* Metric Explorer correct loading of metric by URL Id

Signed-off-by: Peter Fitzgibbons <[email protected]>
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 234 lines in your changes are missing coverage. Please review.

Comparison is base (11ecfa6) 56.42% compared to head (99a918c) 57.44%.
Report is 1 commits behind head on 2.x.

Files Patch % Lines
...lic/components/metrics/top_menu/metrics_export.tsx 55.62% 67 Missing ⚠️
public/components/custom_panels/helpers/utils.tsx 26.53% 36 Missing ⚠️
...blic/components/custom_panels/redux/panel_slice.ts 18.91% 30 Missing ⚠️
...c/components/metrics/redux/slices/metrics_slice.ts 80.64% 17 Missing and 1 partial ⚠️
...components/metrics/sidebar/metrics_edit_inline.tsx 31.81% 15 Missing ⚠️
public/components/metrics/view/metrics_grid.tsx 69.44% 11 Missing ⚠️
...isualization_container/visualization_container.tsx 68.96% 9 Missing ⚠️
...ks/components/paragraph_components/para_output.tsx 88.05% 8 Missing ⚠️
public/components/visualizations/visualization.tsx 45.45% 6 Missing ⚠️
public/components/common/search/search.tsx 0.00% 5 Missing ⚠️
... and 13 more
Additional details and impacted files
@@            Coverage Diff             @@
##              2.x    #1309      +/-   ##
==========================================
+ Coverage   56.42%   57.44%   +1.02%     
==========================================
  Files         327      330       +3     
  Lines       11930    12094     +164     
  Branches     2740     2793      +53     
==========================================
+ Hits         6732     6948     +216     
+ Misses       5153     5100      -53     
- Partials       45       46       +1     
Flag Coverage Δ
dashboards-observability 57.44% <61.76%> (+1.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -43,10 +43,12 @@ export const Sidebar = ({

useEffect(() => {
if (additionalMetric) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's an additional metric?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A badly named variable.

It's used when navigating to a specific metric for "edit", the URL is ./metrics/{savedObjectId}.

I tried a bunch fo names that didn't work and then just gave up trying. (Not the best approach)

@pjfitzgibbons pjfitzgibbons merged commit fd55726 into 2.x Dec 19, 2023
12 of 28 checks passed
@pjfitzgibbons pjfitzgibbons deleted the bugfix/metric-loading-loop branch December 19, 2023 20:17
@ps48
Copy link
Member

ps48 commented Dec 19, 2023

@pjfitzgibbons Is this already merged to main or are we pending a forward port? @kavithacm @YANG-DB can you please make sure the accurate backport labels are in before you review the PRs? We have missed the backports before that have caused last minute merge issues during the release timeframe

@pjfitzgibbons
Copy link
Contributor Author

Yes, missing forward backport (ugh, the terminology!)

@opensearch-trigger-bot
Copy link
Contributor

The backport to main failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-main main
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-main
# Create a new branch
git switch --create backport/backport-1309-to-main
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fd55726896e9785d0a9a352b520df0e7cb16d1e5
# Push it to GitHub
git push --set-upstream origin backport/backport-1309-to-main
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-main

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

@ps48
Copy link
Member

ps48 commented Dec 19, 2023

Yes, missing forward backport (ugh, the terminology!)

OR forward backport main 🤣

pjfitzgibbons added a commit that referenced this pull request Dec 20, 2023
Signed-off-by: Peter Fitzgibbons <[email protected]>
pjfitzgibbons added a commit that referenced this pull request Dec 21, 2023
Signed-off-by: Peter Fitzgibbons <[email protected]>
amsiglan pushed a commit to amsiglan/dashboards-observability that referenced this pull request Jun 7, 2024
* Metric workflow fixes

* Visualization action-menu correction for OSD Dashboards context and Menu Explorer context
* Visualization of Promql metric navigate to Metric Explorer on "Edit"
* Metric Explorer correct loading of metric by URL Id

---------

Signed-off-by: Peter Fitzgibbons <[email protected]>
(cherry picked from commit fd55726)
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.

5 participants