Atm monitoring UI #815

th3M1ke · 2021-09-22T07:44:18Z

Which problem is this PR solving?

Add Monitoring tab for ATM Create a monitor tab for ATM #753

Short description of the changes

Functional logic and unit tests 🙂

albertteoh · 2021-09-22T11:11:46Z

@th3M1ke please sign your commits.

codecov · 2021-09-22T14:39:28Z

Codecov Report

Merging #815 (a041529) into master (be446bc) will increase coverage by 0.12%.
The diff coverage is 99.15%.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
+ Coverage   95.31%   95.44%   +0.12%     
==========================================
  Files         231      240       +9     
  Lines        7103     7458     +355     
  Branches     1709     1811     +102     
==========================================
+ Hits         6770     7118     +348     
- Misses        327      334       +7     
  Partials        6        6

Impacted Files	Coverage Δ
packages/jaeger-ui/src/components/App/index.js	`100.00% <ø> (ø)`
...s/jaeger-ui/src/utils/redux-form-field-adapter.tsx	`100.00% <ø> (ø)`
packages/jaeger-ui/src/components/App/TopNav.tsx	`91.66% <50.00%> (-2.46%)`	⬇️
...c/components/Monitor/ServicesView/serviceGraph.tsx	`98.07% <98.07%> (ø)`
...r-ui/src/components/Monitor/ServicesView/index.tsx	`98.71% <98.71%> (ø)`
packages/jaeger-ui/src/actions/jaeger-api.js	`100.00% <100.00%> (ø)`
packages/jaeger-ui/src/api/jaeger.js	`100.00% <100.00%> (ø)`
...ger-ui/src/components/Monitor/EmptyState/index.tsx	`100.00% <100.00%> (ø)`
...nitor/ServicesView/operationDetailsTable/index.tsx	`100.00% <100.00%> (ø)`
...or/ServicesView/operationDetailsTable/opsGraph.tsx	`100.00% <100.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be446bc...a041529. Read the comment docs.

Ashmita152 · 2021-09-27T12:31:02Z

I tried testing this with @albertteoh's ATM docker-compose today. It worked like charm 🎉 🎉 . Screenshots to get the look and feel. These are from /monitoring endpoint.

Great work @th3M1ke 🥳 🍾

albertteoh · 2021-09-28T07:50:26Z

@rubenvp8510 @yoave23 would you be able to take a look over this PR if you get a chance, please (I'm pretty inexperienced with react)?

rubenvp8510 · 2021-09-29T04:14:14Z

packages/jaeger-ui/src/components/Monitoring/EmptyState/index.tsx

+            header={<h2 className="ub-m0">Get started with Services Monitoring</h2>}
+            footer={
+              <Button
+                style={{ backgroundColor: '#199', color: '#fff' }}


Can we use an style for this? or there is any reason for put this directly on the style attribute?

rubenvp8510 · 2021-09-29T04:54:25Z

Small optional comment! overall Looks good! Awesome work!

yoave23

looks great 👍
some minor comments inside.

packages/jaeger-ui/src/actions/jaeger-api.js

yoave23 · 2021-09-29T08:53:16Z

packages/jaeger-ui/src/components/App/TopNav.tsx

@@ -68,13 +69,22 @@ if (getConfigValue('qualityMetrics.menuEnabled')) {
  });
 }

+if (getConfigValue('monitoring.menuEnabled')) {


need to document it somewhere

yoave23 · 2021-09-29T09:00:36Z

packages/jaeger-ui/src/components/Monitoring/ServicesView/index.test.js

+
+const props = mapStateToProps(state);
+
+Date.now = jest.fn(() => 1487076708000);


please move the date to a constant with a meaningful name

yoave23 · 2021-09-29T09:01:56Z

packages/jaeger-ui/src/components/Monitoring/ServicesView/index.tsx

+import { ActionFunction, Action } from 'redux-actions';
+// @ts-ignore
+import { Field, formValueSelector, reduxForm } from 'redux-form';
+// @ts-ignore


don't think you need to ignore this one

packages/jaeger-ui/src/components/Monitoring/ServicesView/index.tsx

yoave23 · 2021-09-29T09:09:17Z

packages/jaeger-ui/src/components/Monitoring/ServicesView/index.tsx

+                style={{ fontSize: 14, width: 218 }}
+                value={this.state.searchOps}
+                disabled={metrics.operationMetricsLoading === true}
+                onChange={a => {


please use something more meaningful than a

yoave23 · 2021-09-29T09:13:22Z

packages/jaeger-ui/src/components/Monitoring/ServicesView/operationDetailsTable/index.test.js

+  endTime: 1632133918915,
+  lookback: 3600 * 1000,
+  serviceName: 'serviceName',
+  // state


th3M1ke · 2021-10-06T11:16:05Z

Closing PR just for rerun pipeline

jpkrohling · 2021-10-06T12:44:41Z

@th3M1ke, is this ready for re-review? We have a release going out today, but I think it's too late to include this, right?

th3M1ke · 2021-10-06T12:49:27Z

Yes, it's ready for re-review, I believe. I got approves from @yoave23 and @rubenvp8510, but they were reset because of a new commit with code coverage increase. Also we plan to review performance this Friday with @albertteoh.

albertteoh · 2021-10-06T12:57:38Z

This is looking great @th3M1ke!

The following are just a few things I noticed while playing around with it:

The text in the "Impact" column's "info" is wrapping in the middle of words. It would be better to wrap on the word boundaries:

How will a user know what percentile the latency column is displaying? I'm assuming it's 95th percentile based on the colour scheme

The horizontal alignment for the lookback seems a little off from both the service dropdown and the "Aggregation of..." text. I feel it should align with either one of these, perhaps in line with the service dropdown.

I like the thoughtfulness of how the label appears on the side of the graph with the most real estate. A few things I wanted to point out (I can appreciate some of these can be quite tricky to implement, and also a little subjective, and so probably not critical for version 1):

I think the units would be helpful, either in the graph label or in the title (ms).
I feel that the percentiles in the label should be in reverse order: 95th first, then 75th and finally 50th. That way, the labels align more naturally to each line in the graph. At the moment, my brain needs to reverse the order of what I read in the label with what I see in the graph. :)
I prefer little dot markers highlighting the exact time point on each line where the vertical "cross-hair" is.
It would be nice to have the time markers at rounded times. For example, below, there's 23:23 then 23:31, but maybe it makes more sense to have say 23:20, 23:25 and 23:30 or even just 23:20 and 23:30.

It would be great if we could see the specific time in the label as well, for example (from Grafana):

It would be nice if the headers of the service-level and operation-level graphs are consistent:

I tried clicking on the "View traces" button but it isn't able to select the Operation because the dropdown isn't populated on initial load, but only when I re-select "redis" again. Not sure if this is an existing bug in Jaeger UI. The service is definitely auto-selected, so I would have the Operations dropdown should be populated too.

nit: If I click on the up-arrow and down-arrows of each column, it seems to have mixed behaviours between columns. Also, sometimes down-arrow toggles ascending and descending order, and sometimes it just sticks to descending order. It would be good if there was consistent behaviour across all the columns.

nit: If I select 2 days lookback, it shows 48 hours which is correct, but would be nicer if it also shows "2 days" to be consistent with the selected lookback text.

It would be good to include the units somewhere, perhaps in the label (req/s), as is done in the per operation table.

albertteoh · 2021-10-06T13:06:16Z

We have a release going out today, but I think it's too late to include this, right?

Sorry, my bad for the slow turnaround on reviews to miss the release cut-off. Probably too late to make it for today's release.

th3M1ke · 2021-10-28T15:51:40Z

The text in the "Impact" column's "info" is wrapping in the middle of words. It would be better to wrap on the word boundaries:

Done

How will a user know what percentile the latency column is displaying? I'm assuming it's 95th percentile based on the colour scheme

Units was added

The horizontal alignment for the lookback seems a little off from both the service dropdown and the "Aggregation of..." text. I feel it should align with either one of these, perhaps in line with the service dropdown.

Fixed. Aligned with text

I like the thoughtfulness of how the label appears on the side of the graph with the most real estate. A few things I wanted to point out (I can appreciate some of these can be quite tricky to implement, and also a little subjective, and so probably not critical for version 1):

I think the units would be helpful, either in the graph label or in the title (ms).

done

I feel that the percentiles in the label should be in reverse order: 95th first, then 75th and finally 50th. That way, the labels align more naturally to each line in the graph. At the moment, my brain needs to reverse the order of what I read in the label with what I see in the graph. :)

done

I prefer little dot markers highlighting the exact time point on each line where the vertical "cross-hair" is.

unfortunately react-visdoes not provide this functionality 😞

It would be nice to have the time markers at rounded times. For example, below, there's 23:23 then 23:31, but maybe it makes more sense to have say 23:20, 23:25 and 23:30 or even just 23:20 and 23:30.

cant find this functionality in react-vis

It would be great if we could see the specific time in the label as well, for example (from Grafana):

done

It would be nice if the headers of the service-level and operation-level graphs are consistent:

Done

I tried clicking on the "View traces" button but it isn't able to select the Operation because the dropdown isn't populated on initial load, but only when I re-select "redis" again. Not sure if this is an existing bug in Jaeger UI. The service is definitely auto-selected, so I would have the Operations dropdown should be populated too.

This bug is not related to changes introduced in this PR. You can reproduce it if you search for some specific operation, copy generated URL and past it into incognito window

nit: If I click on the up-arrow and down-arrows of each column, it seems to have mixed behaviours between columns. Also, sometimes down-arrow toggles ascending and descending order, and sometimes it just sticks to descending order. It would be good if there was consistent behaviour across all the columns.

This is library's(antd) behaviour

nit: If I select 2 days lookback, it shows 48 hours which is correct, but would be nicer if it also shows "2 days" to be consistent with the selected lookback text.

Done.

It would be good to include the units somewhere, perhaps in the label (req/s), as is done in the per operation table.

Done

albertteoh · 2021-10-29T12:39:12Z

EDIT: I've clarified what I meant by inlining suggestions in code.

Thanks @th3M1ke! I have a few comments, but I think we can discuss/address them in a follow-up PR if it's too much effort to address here.

I feel that the percentiles in the label should be in reverse order: 95th first, then 75th and finally 50th. That way, the labels align more naturally to each line in the graph. At the moment, my brain needs to reverse the order of what I read in the label with what I see in the graph. :)

done

It looks like the percentile labels are still in ascending order.

albertteoh · 2021-10-29T13:38:37Z

Please rebase and address code coverage issues.

albertteoh

Thought it would be clearer to enter my suggestions directly inline to the code.

packages/jaeger-ui/src/components/Monitoring/ServicesView/index.tsx

packages/jaeger-ui/src/components/Monitoring/ServicesView/operationDetailsTable/index.tsx

packages/jaeger-ui/src/components/Monitoring/ServicesView/index.tsx

packages/jaeger-ui/src/components/Monitoring/ServicesView/operationDetailsTable/index.tsx

packages/jaeger-ui/src/components/Monitoring/ServicesView/serviceGraph.tsx

albertteoh · 2021-11-05T11:24:33Z

CI jobs seem stuck. Closing and reopening PR to re-trigger build.

albertteoh · 2021-11-10T10:16:11Z

Reopened PR to restart stuck build pipeline.

albertteoh

Looks great! 👍

When using the test setup in docker-compose/monitor I noticed that the HTTP GET operation was missing for the driver service in the monitor page, but present when I click on view traces. Do you know why?

th3M1ke · 2021-11-15T09:52:34Z

Good catch, @albertteoh !

I've checked, but I don't know why I did not received it from jaeger-query...

Request to get data is GET /api/metrics/latencies?service=driver&endTs=1636969517360&groupByOperation=true&lookback=7200000&quantile=0.95&ratePer=3600000&step=60000

Is it correct?

albertteoh

Is it correct?

Yeah, looks correct to me, it could be a bug on jaeger-query's side.
I've created a ticket for myself to investigate: jaegertracing/jaeger#3392

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

albertteoh

LGTM

th3M1ke requested review from everett980, rubenvp8510 and tiffon as code owners September 22, 2021 07:44

th3M1ke force-pushed the atm-monitoring-ui branch from 5bc11e7 to deab23c Compare September 22, 2021 14:23

albertteoh mentioned this pull request Sep 28, 2021

"Monitor" tab for service health metrics jaegertracing/jaeger#2954

Closed

14 tasks

rubenvp8510 reviewed Sep 29, 2021

View reviewed changes

rubenvp8510 previously approved these changes Sep 29, 2021

View reviewed changes

yoave23 previously approved these changes Sep 29, 2021

View reviewed changes

th3M1ke dismissed stale reviews from yoave23 and rubenvp8510 via a4ba580 October 1, 2021 16:40

th3M1ke closed this Oct 6, 2021

th3M1ke reopened this Oct 6, 2021

th3M1ke force-pushed the atm-monitoring-ui branch from a234513 to 0942781 Compare October 29, 2021 09:56

albertteoh reviewed Oct 29, 2021

View reviewed changes

packages/jaeger-ui/src/components/Monitoring/ServicesView/serviceGraph.tsx Outdated Show resolved Hide resolved

th3M1ke force-pushed the atm-monitoring-ui branch 2 times, most recently from 9a86bb8 to d471027 Compare November 5, 2021 11:09

albertteoh previously approved these changes Nov 5, 2021

View reviewed changes

albertteoh closed this Nov 5, 2021

albertteoh reopened this Nov 5, 2021

th3M1ke dismissed albertteoh’s stale review via 12ae5fd November 8, 2021 11:05

th3M1ke force-pushed the atm-monitoring-ui branch from 12ae5fd to 22fd58c Compare November 8, 2021 12:12

albertteoh closed this Nov 10, 2021

albertteoh reopened this Nov 10, 2021

th3M1ke force-pushed the atm-monitoring-ui branch 2 times, most recently from 784db4c to 0f52a3b Compare November 10, 2021 17:58

th3M1ke closed this Nov 10, 2021

th3M1ke reopened this Nov 10, 2021

th3M1ke force-pushed the atm-monitoring-ui branch 3 times, most recently from d403411 to 79caab0 Compare November 12, 2021 12:45

th3M1ke requested review from rubenvp8510 and albertteoh November 12, 2021 21:37

albertteoh reviewed Nov 13, 2021

View reviewed changes

th3M1ke force-pushed the atm-monitoring-ui branch from 72bbc7f to 63c7922 Compare November 15, 2021 11:17

albertteoh mentioned this pull request Nov 15, 2021

Investigate missing operation from jaeger-query metrics reader jaegertracing/jaeger#3392

Closed

albertteoh reviewed Nov 15, 2021

View reviewed changes

ATM Monitor UI

a041529

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke force-pushed the atm-monitoring-ui branch from 63c7922 to a041529 Compare November 15, 2021 11:55

th3M1ke closed this Nov 15, 2021

th3M1ke reopened this Nov 15, 2021

th3M1ke requested a review from albertteoh November 15, 2021 13:26

albertteoh approved these changes Nov 15, 2021

View reviewed changes

albertteoh merged commit 9068a11 into jaegertracing:master Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atm monitoring UI #815

Atm monitoring UI #815

th3M1ke commented Sep 22, 2021 •

edited

Loading

albertteoh commented Sep 22, 2021

codecov bot commented Sep 22, 2021 •

edited

Loading

Ashmita152 commented Sep 27, 2021

albertteoh commented Sep 28, 2021

rubenvp8510 Sep 29, 2021

rubenvp8510 commented Sep 29, 2021 •

edited

Loading

yoave23 left a comment

yoave23 Sep 29, 2021

yoave23 Sep 29, 2021

yoave23 Sep 29, 2021

yoave23 Sep 29, 2021

yoave23 Sep 29, 2021

th3M1ke commented Oct 6, 2021

jpkrohling commented Oct 6, 2021

th3M1ke commented Oct 6, 2021

albertteoh commented Oct 6, 2021 •

edited

Loading

albertteoh commented Oct 6, 2021

th3M1ke commented Oct 28, 2021

albertteoh commented Oct 29, 2021 •

edited

Loading

albertteoh commented Oct 29, 2021

albertteoh left a comment

albertteoh commented Nov 5, 2021

albertteoh commented Nov 10, 2021

albertteoh left a comment

th3M1ke commented Nov 15, 2021

albertteoh left a comment

albertteoh left a comment


		const props = mapStateToProps(state);

		Date.now = jest.fn(() => 1487076708000);

Atm monitoring UI #815

Atm monitoring UI #815

Conversation

th3M1ke commented Sep 22, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

albertteoh commented Sep 22, 2021

codecov bot commented Sep 22, 2021 • edited Loading

Codecov Report

Ashmita152 commented Sep 27, 2021

albertteoh commented Sep 28, 2021

rubenvp8510 Sep 29, 2021

Choose a reason for hiding this comment

rubenvp8510 commented Sep 29, 2021 • edited Loading

yoave23 left a comment

Choose a reason for hiding this comment

yoave23 Sep 29, 2021

Choose a reason for hiding this comment

yoave23 Sep 29, 2021

Choose a reason for hiding this comment

yoave23 Sep 29, 2021

Choose a reason for hiding this comment

yoave23 Sep 29, 2021

Choose a reason for hiding this comment

yoave23 Sep 29, 2021

Choose a reason for hiding this comment

th3M1ke commented Oct 6, 2021

jpkrohling commented Oct 6, 2021

th3M1ke commented Oct 6, 2021

albertteoh commented Oct 6, 2021 • edited Loading

albertteoh commented Oct 6, 2021

th3M1ke commented Oct 28, 2021

albertteoh commented Oct 29, 2021 • edited Loading

albertteoh commented Oct 29, 2021

albertteoh left a comment

Choose a reason for hiding this comment

albertteoh commented Nov 5, 2021

albertteoh commented Nov 10, 2021

albertteoh left a comment

Choose a reason for hiding this comment

th3M1ke commented Nov 15, 2021

albertteoh left a comment

Choose a reason for hiding this comment

albertteoh left a comment

Choose a reason for hiding this comment

th3M1ke commented Sep 22, 2021 •

edited

Loading

codecov bot commented Sep 22, 2021 •

edited

Loading

rubenvp8510 commented Sep 29, 2021 •

edited

Loading

albertteoh commented Oct 6, 2021 •

edited

Loading

albertteoh commented Oct 29, 2021 •

edited

Loading