mgr/dashboard: fix prometheus api issues on landing page v3 #50214

nizamial09 · 2023-02-22T16:56:54Z

When no prometheus is configured in the cluster, it gives out error
while polling to the prometheus endpoint. so a proper check needs to be
added there.

only fetch the alerts when you toggle the alert viewer. don't fetch it
every 5 second cause its going to cause a heavy toll on the api.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

Pegonzal

LGTM @nizamial09 !

Thanks for taking care of it.

Just a couple of minr things:

On the first time opening the page the alerts get mixed up after a short period of tyme of them loading:

Pegonzal · 2023-02-22T23:18:48Z

src/pybind/mgr/dashboard/frontend/src/app/shared/helpers/helpers.module.ts

+import { NgModule } from '@angular/core';
+import { CommonModule } from '@angular/common';
+
+@NgModule({
+ declarations: [],
+ imports: [CommonModule]
+})
+export class HelpersModule {}


Is this module being used?

I couldn't find a place to put the prometheus-helpers anywhere so i created this helper module to put the prometheus-list-helper class inside. Although we are not directly accessing the module, the prometheus-list-helper gets extended to the DashboardComponent and couple of others as well.

nizamial09 · 2023-02-23T06:45:05Z

On the first time opening the page the alerts get mixed up after a short period of tyme of them loading

@Pegonzal i have not seen this in mine. checked a couple of time but couldn't reproduce. is this recurring for you?

Pegonzal · 2023-02-23T09:14:15Z

On the first time opening the page the alerts get mixed up after a short period of tyme of them loading

@Pegonzal i have not seen this in mine. checked a couple of time but couldn't reproduce. is this recurring for you?

@nizamial09 this was happening to me when first opening the page but only once, and it would get fixed after a few seconds when all of the alerts had properly loaded, or so it semeed, so it is nothing to worry about, just wanted to mention it in case it it was part of another bigger issue, but I didn't find anything else

When no prometheus is configured in the cluster, it gives out error while polling to the prometheus endpoint. so a proper check needs to be added there. Also some improvements to the exisiting behaviour Fixes: https://tracker.ceph.com/issues/58867 Signed-off-by: Nizamudeen A <nia@redhat.com>

only fetch the alerts when you toggle the alert viewer. don't fetch it every 5 second cause its going to cause a heavy toll on the api. Fixes: https://tracker.ceph.com/issues/58867 Signed-off-by: Nizamudeen A <nia@redhat.com>

epuertat · 2023-02-28T11:25:09Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/new-dashboard/dashboard/dashboard.component.ts


 @Component({
 selector: 'cd-dashboard',
 templateUrl: './dashboard.component.html',
 styleUrls: ['./dashboard.component.scss']
 })
-export class DashboardComponent implements OnInit, OnDestroy {
+export class DashboardComponent extends PrometheusListHelper implements OnInit, OnDestroy {


First of all, I really love when I see some OOP stuff going on. That said, DashboardComponent is an Angular Component, and PrometheusHelper is an Angular Directive (with no selector, so it can't be used as Directive either) and which inherits from ListWithDetails, which in turn brings properties like expandedRow or tableStatus...

I think this is a bit messy inheritance model...

What do you want from PrometheusHelper? Some way to run code based on the online status of a backend component... For that, in some places we're using Status Guards (hooks in the Angular routing lifecycle to enable/activate pages based on conditions). However, that doesn't fit here.

So part of the issue is that we have to consume 2 service methods:

IsPrometheusConfigured(): this offloads the internal state of the service to the consumer component, which is not polite (consumers shouldn't have to worry about the internal state of anything else that themselves')

getPrometheusData()

If a devel just want to fetch Prometheus data, why forcing them to first check if Prometheus is configured? Let's follow the Python "better ask for forgiveness..." and detect that condition, either during the service instantiation or during the service method:

@Injectable({ providedIn: 'root' }) export class PrometheusService { private baseURL = 'api/prometheus'; private settingsKey = { alertmanager: 'api/settings/alertmanager-api-host', prometheus: 'api/settings/prometheus-api-host' }; isPrometheusConfigured = false; constructor(private http: HttpClient, private settingsService: SettingsService) { this.isPrometheusConfigured$ = this.prometheusService.ifPrometheusConfigured().share(); } getPrometheusData(params: any): any { return concat( this.isPrometheusConfigured$, this.http.get<any>(`${this.baseURL}/data`, { params }) ); }

BTW the example above is not 100% functiona, the 2nd subscription still needs to check for the Prometheus configured status.

the prometheus-list.helper was already there but it was only accessible to prometheus component. So what i did is to move that component to shared. I'll refactor this code, and it'll be affecting several other components. i'll check it.

@epuertat for some days I've been trying this and i couldn't fully convert the existing behaviour. The original ifPrometheusConfigured() function doesn't returns a subscribable object. but even after making the changes that is needed to make it subscribable it doesn't work as it is before.

epuertat · 2023-02-28T11:28:18Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/new-dashboard/dashboard/dashboard.component.ts

+ this.prometheusService.ifPrometheusConfigured(() => {
+ if (this.timerGetPrometheusDataSub) {
+ this.timerGetPrometheusDataSub.unsubscribe();
+ }
+ this.timerGetPrometheusDataSub = timer(0, this.timerTime).subscribe(() => {
+ selectedTime = this.updateTimeStamp(selectedTime);
+
+ for (const queryName in queries) {
+ if (queries.hasOwnProperty(queryName)) {
+ const query = queries[queryName];
+ let interval = selectedTime.step;
+
+ if (query.includes('rate') && selectedTime.step < 20) {
+ interval = 20;
+ } else if (query.includes('rate')) {
+ interval = selectedTime.step * 2;
+ }
+
+ const intervalAdjustedQuery = query.replace(/\[(.*?)\]/g, `[${interval}s]`);
+
+ this.prometheusService
+ .getPrometheusData({
+ params: intervalAdjustedQuery,
+ start: selectedTime['start'],
+ end: selectedTime['end'],
+ step: selectedTime['step']
+ })
+ .subscribe((data: any) => {
+ if (data.result.length) {
+ this.queriesResults[queryName] = data.result[0].values;
+ }
+ });


Most of this code is unrelated to the Dashboard presentation and rather about processing Prometheus data, so why not having this encapsulated in the proper Prometheus service?

Not sure how 'customized' the queries are, but if they are simple queries, we could try and perhaps fetch multiple timeseries in a single query.

@Pegonzal since you worked the most here I will need your help with this. What do you think about this?

Most of this code is unrelated to the Dashboard presentation and rather about processing Prometheus data, so why not having this encapsulated in the proper Prometheus service?

I could look into cleaning it up a bit, and refactor it into the prometheus service.

Not sure how 'customized' the queries are, but if they are simple queries, we could try and perhaps fetch multiple timeseries in a single query.

I was looking into it and it might work, thanks Ernesto. I'll try to have them all together into one query to optimize it.

If you want @nizamial09 we can create a tracker and I'll take care of it, on a separate PR.

yup, i'll open a separate tracker and will assign it to you,

https://tracker.ceph.com/issues/58936

nizamial09 · 2023-03-14T12:52:11Z

jenkins test make check

nizamial09 requested a review from Pegonzal February 22, 2023 16:56

nizamial09 requested a review from a team as a code owner February 22, 2023 16:56

nizamial09 requested review from aaSharma14 and removed request for a team February 22, 2023 16:56

github-actions bot added dashboard pybind labels Feb 22, 2023

nizamial09 force-pushed the landing-page-prom-error-fix branch from 1588b27 to 8cba36e Compare February 22, 2023 16:59

nizamial09 requested review from a team as code owners February 22, 2023 16:59

github-actions bot added the documentation label Feb 22, 2023

nizamial09 force-pushed the landing-page-prom-error-fix branch from 8cba36e to 1393a31 Compare February 22, 2023 17:00

nizamial09 removed the documentation label Feb 22, 2023

nizamial09 removed request for a team February 22, 2023 17:00

nizamial09 force-pushed the landing-page-prom-error-fix branch from 1393a31 to ef52d00 Compare February 22, 2023 17:05

Pegonzal approved these changes Feb 22, 2023

View reviewed changes

nizamial09 added 2 commits February 28, 2023 13:49

mgr/dashboard: improve the logic on the alert viewer

6cb0679

only fetch the alerts when you toggle the alert viewer. don't fetch it every 5 second cause its going to cause a heavy toll on the api. Fixes: https://tracker.ceph.com/issues/58867 Signed-off-by: Nizamudeen A <nia@redhat.com>

nizamial09 force-pushed the landing-page-prom-error-fix branch from ef52d00 to 6cb0679 Compare February 28, 2023 08:20

epuertat reviewed Feb 28, 2023

View reviewed changes

nizamial09 merged commit 4ca0190 into ceph:main Mar 15, 2023

nizamial09 deleted the landing-page-prom-error-fix branch March 15, 2023 06:41

This was referenced Mar 15, 2023

reef: mgr/dashboard: fix prometheus api issues on landing page v3 #50541

Merged

quincy: mgr/dashboard: Landing page v3 #50608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr/dashboard: fix prometheus api issues on landing page v3 #50214

mgr/dashboard: fix prometheus api issues on landing page v3 #50214

nizamial09 commented Feb 22, 2023

Pegonzal left a comment

Pegonzal Feb 22, 2023

nizamial09 Feb 23, 2023

nizamial09 commented Feb 23, 2023

Pegonzal commented Feb 23, 2023

epuertat Feb 28, 2023

epuertat Feb 28, 2023

nizamial09 Feb 28, 2023

nizamial09 Mar 8, 2023

epuertat Feb 28, 2023

nizamial09 Mar 1, 2023

Pegonzal Mar 7, 2023

nizamial09 Mar 8, 2023

nizamial09 Mar 9, 2023

nizamial09 commented Mar 14, 2023

mgr/dashboard: fix prometheus api issues on landing page v3 #50214

mgr/dashboard: fix prometheus api issues on landing page v3 #50214

Conversation

nizamial09 commented Feb 22, 2023

Contribution Guidelines

Checklist

Pegonzal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nizamial09 commented Feb 23, 2023

Pegonzal commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nizamial09 commented Mar 14, 2023