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

[O11y][Nagios XI] Lens migration for visualizations to Kibana version 8.3.0 #5573

Merged

Conversation

rajvi-patel-22
Copy link
Contributor

  • Enhancement

What does this PR do?

  • Migrate visualizations to lens for Nagios XI integration package

  • Statistics for Nagios XI Lens migration:

Nagios XI dashboard Before Migration   After Migration  
  Lens Visualization Lens Visualization
[Metrics Nagios XI] Hosts Dashboard 6 1 6 1
[Metrics Nagios XI] Overview 5 1 5 1
[Metrics Nagios XI] Events Dashboard 0 1 0 1
[Metrics Nagios XI] Services Dashboard 10 2 10 1
  21 5 21 4

Note: One visualization is migrated to the new control panels. Count of visualization is 4 after migration because all the dashboards have 1 markdown visualization whose alternative is not available in Lens hence we have not migrated those.

Checklist

  • I have added an entry to my package's changelog.yml file.
  • I have verified that panels are populated with data.
  • I have verified that panels are not distorted after being migrated to lens.
  • I have updated screenshots of dashboard.
  • I have verified that data count are matching and panel aggregations are same as before.

Author's Checklist

  • Migrated panels should be removed from visualization folder.
  • Migrated visualizations are populating in current Kibana version 8.3.0 itself.

Related issues

Issues Identified

  • In input control visualization, we can make dependency filters that strictly depend on each other but in new controls, dependency filtering is possible but filters are not required to be strict. As per the discussion, this is the expected behavior of new controls.

@rajvi-patel-22 rajvi-patel-22 requested a review from a team as a code owner March 17, 2023 10:51
@elasticmachine
Copy link

elasticmachine commented Mar 17, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-03-17T10:52:36.120+0000

  • Duration: 16 min 27 sec

Test stats 🧪

Test Results
Failed 0
Passed 40
Skipped 0
Total 40

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (3/3) 💚 3.432
Classes 100.0% (3/3) 💚 3.432
Methods 97.059% (33/34) 👍 5.899
Lines 95.23% (1138/1195) 👍 3.848
Conditionals 100.0% (0/0) 💚

Copy link
Collaborator

@kush-elastic kush-elastic left a comment

Choose a reason for hiding this comment

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

LGTM!

@agithomas
Copy link
Contributor

Please revisit the color usage for the below viz

image

@agithomas
Copy link
Contributor

Encourage you to create Smaller PRs going forward - One PR for either one dashboard / datastream.

There are two visualisation files but only one screenshot image updated. Please confirm on the number of modified dashboards

@rajvi-patel-22
Copy link
Contributor Author

Please revisit the color usage for the below viz

image

@agithomas, In this integration we have only migrated one visualization to new control panels. Apart from that we haven't touched any panels. The functionality/color palette of those panels are same as before. This is just the lens migration PR so we won't be changing any behavior/files as a part of this PR otherwise it will mess it up. However, If you strongly feel that this change needs to be done then we can raise a separate issue for that and track them separately.

@rajvi-patel-22
Copy link
Contributor Author

Encourage you to create Smaller PRs going forward - One PR for either one dashboard / datastream.

There are two visualisation files but only one screenshot image updated. Please confirm on the number of modified dashboards

@agithomas, I understand the number of changed files is quite big but only one visualization is migrated. This integration is not yet converted to by reference and because of that there is a separate file for each panel. When we are migrating dashboards to the newer versions, the new properties are being added and the version related properties (e.g., coreMigrationVersion and migrationVersion) are also getting updated. This is the reason behind increased number of file changes. Hence, creating different PR for one visualization seems irrational.

Yes, there are two visualization files but that doesn't mean it has two dashboards. The number of dashboards can be determined by the number of files falls under packages/nagios_xi/kibana/dashboard folder. This integration contains four dashbaords but we just updated the screenshot of one dashboard from which the panel was migrated.

@agithomas
Copy link
Contributor

The functionality/color palette of those panels are same as before.

Can you elaborate the colour scheme used in the old dashboard ? Please refer and compare the two panels (circled in blue) in old and new dashboard.

Screenshot 2023-03-20 at 1 19 17 PM

@agithomas
Copy link
Contributor

Hence, creating different PR for one visualization seems irrational.

I hoped you had looked at the comment more carefully. The requirement is to create one PR for one dashboard / one datastream. I didn't ask one PR per visualisation.

There may be cases when one dashboard represents data from multiple datastreams. At certain times, one datastream may have one dashboard. This is the reason why i mentioned - one PR per dashboard / datastream. Are there any challenges following this practise?

@rajvi-patel-22
Copy link
Contributor Author

Can you elaborate the colour scheme used in the old dashboard ? Please refer and compare the two panels (circled in blue) in old and new dashboard.

Screenshot 2023-03-20 at 1 19 17 PM

@agithomas, the screenshot of the [Metrics Nagios XI] Services Dashboard (uploaded with the integration package) does not sync with the Kibana files. Can you please refer to the below screenshot to compare the migrated visualization:

[Metrics Nagios XI] Services Dashboard before migration:
screencapture-storage-0-kb-us-central1-gcp-cloud-es-io-9243-app-dashboards-2023-03-20-14_35_33

[Metrics Nagios XI] Services Dashboard After migration:
nagios_xi-service-dashboard-screenshot

As mentioned in description, only control panels are migrated. Apart from that everything is in lens.
Old Control panels:
image
New control panels:
image

@rajvi-patel-22
Copy link
Contributor Author

Can you elaborate the colour scheme used in the old dashboard ?

Regarding colors in mentioned panels,

The custom colors are used in Swap Usage [Metrics Nagios XI] and Root Partition [Metrics Nagios XI].
image

The default color is used in Swap Usage Gauge [Metrics Nagios XI] and Root Partition Gauge [Metrics Nagios XI].
image

@agithomas
Copy link
Contributor

Thanks for sharing the details.

@agithomas
Copy link
Contributor

Please split the PR to smaller one - One per dashboard preferably.

@agithomas
Copy link
Contributor

@rajvi-elastic , any progress on this PR?

@rajvi-patel-22
Copy link
Contributor Author

@rajvi-elastic , any progress on this PR?

@agithomas, In this integration, only one old control panel has been migrated to the new controls. And splitting this PR into multiple ones (either based on data-stream or dashboard) would be difficult because one file could belong to the both PRs. So we are waiting for your review only.

agithomas
agithomas previously approved these changes Mar 27, 2023
Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM!

@agithomas agithomas dismissed their stale review March 27, 2023 17:32

Need additional clarification on screenshots

@agithomas
Copy link
Contributor

I see that the screenshot that is deleted and the screenshot that is added are totally different. Please explain why?

@rajvi-patel-22
Copy link
Contributor Author

rajvi-patel-22 commented Mar 28, 2023

I see that the screenshot that is deleted and the screenshot that is added are totally different. Please explain why?

@agithomas, Can you please refer to this comment. I have mentioned over there. Please let me know if you need more information for the same.

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM

@kush-elastic kush-elastic merged commit 55d82e1 into elastic:main Mar 28, 2023
@elasticmachine
Copy link

Package nagios_xi - 0.4.0 containing this change is available at https://epr.elastic.co/search?package=nagios_xi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:nagios_xi Nagios XI Team:Service-Integrations Label for the Service Integrations team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants