(SPM) Add span kind selector #2012

albertteoh · 2023-12-02T12:07:00Z

Which problem is this PR solving?

Resolves [Bug]: Empty metrics in the monitoring tab jaeger#4035
Related to Use all span kinds as default for metrics jaeger#3898

Description of the changes

Add a span kind filter to the Monitor tab.
As span kinds are an attribute of a span, much like the service name, I thought it made sense to add the dropdown filter next to the service filter.
I'm not a designer so open to ideas on how to make this better!

How was this change tested?

Manually tested as per the following screen recording.
Update tests.

Screen.Recording.2023-12-02.at.10.37.29.pm.mov

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

codecov · 2023-12-02T12:10:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5855da2) 96.54% compared to head (ff1ab7b) 96.55%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2012   +/-   ##
=======================================
  Coverage   96.54%   96.55%           
=======================================
  Files         256      256           
  Lines        7613     7625   +12     
  Branches     1978     1982    +4     
=======================================
+ Hits         7350     7362   +12     
  Misses        263      263

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

albertteoh · 2023-12-02T12:10:28Z

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

+  { label: 'Client', value: 'client' },
+  { label: 'Server', value: 'server' },
+  { label: 'Internal', value: 'internal' },
+  { label: 'Producer', value: 'producer' },
+  { label: 'Consumer', value: 'consumer' },


Label naming follows convention used by OpenTelemetry.

yurishkuro · 2023-12-02T17:43:33Z

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

@@ -287,6 +304,31 @@ export class MonitorATMServicesViewImpl extends React.PureComponent<TPropsWithIn
                ))}
              </Field>
            </Col>
+            <Col span={6}>
+              <h2 className="span-kind-selector-header">Choose kind</h2>


Suggested change

<h2 className="span-kind-selector-header">Choose kind</h2>

<h2 className="span-kind-selector-header">Span Kind</h2>

Similarly, s/Choose service/Service/ in L287, using the word "choose" in the UX is redundant

Removed "Choose" in: 07e3c26

yurishkuro · 2023-12-02T17:44:57Z

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

+                }}
+                props={{
+                  className: 'span-kind-selector',
+                  defaultValue: spanKindOptions[0],


also, better not to depend on the array order.

👍🏼 Done in: 46989de

yurishkuro · 2023-12-02T17:47:02Z

packages/jaeger-ui/src/types/metrics.tsx

 export type MetricsAPIQueryParams = {
  quantile: number;
  groupByOperation?: boolean;
  endTs?: number;
  lookback?: number;
  step?: number;
  ratePer?: number;
-  spanKind?: 'unspecified' | 'internal' | 'server' | 'client' | 'producer' | 'consumer';
+  spanKind?: spanKinds;


lots of optional fields here - are they really optional? Even if they are from the server perspective, wouldn't UI code always populate them?

Hmmm.. yeah, not sure why it was done that way initially; removed optionality in: ccd6cc7.

yurishkuro · 2023-12-02T17:49:15Z

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

@@ -361,6 +361,7 @@ describe('mapStateToProps()', () => {
      metrics: originInitialState,
      services: [],
      selectedService: 's1',
+      selectedSpanKind: 'server',


do we have behavioral tests? It would be good to have tests simulating a change in span kind

Yup, I could do better with test coverage; added behavioural tests in: 62a71da.

Signed-off-by: Albert Teoh <albert@packsmith.io>

albertteoh · 2023-12-03T04:56:56Z

Note that I've retitled this PR to use SPM instead of ATM because that's the acronym used in our documentation. If you're okay with that, do you think we should also do the same in our codebase?

yurishkuro · 2023-12-03T18:15:27Z

Yes we should use the same SPM name and clean up the code

yurishkuro · 2023-12-03T18:18:11Z

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

@@ -210,6 +225,7 @@ export class MonitorATMServicesViewImpl extends React.PureComponent<TPropsWithIn

    if (currentService) {
      this.endTime = Date.now();
+      store.set('lastAtmSearchSpanKind', selectedSpanKind);


When we change this to SPM, a couple more improvements:

use constants instead of raw strings

keep abbreviations in uppercase, SPM not Spm (but constants should probably all be named in uppercase)

albertteoh · 2023-12-04T11:01:32Z

Thanks for the review!

albertteoh added 6 commits December 2, 2023 18:17

Add span kind selector

f2552cb

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

Rename single word to two words

7b86eca

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

Update snapshots and test

49c0401

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

Merge branch 'main' into add-span-kind-selector

78937f0

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

Fix import formatting

c6bade9

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

yarn prettier

3cb5605

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>

albertteoh requested a review from a team as a code owner December 2, 2023 12:07

albertteoh requested review from jkowall and removed request for a team December 2, 2023 12:07

albertteoh commented Dec 2, 2023

View reviewed changes

albertteoh added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Dec 2, 2023

yurishkuro reviewed Dec 2, 2023

View reviewed changes

yurishkuro changed the title ~~Add span kind selector~~ (ATM) Add span kind selector Dec 2, 2023

Albert Teoh added 7 commits December 3, 2023 07:55

Remove redundant 'Choose' from title

07e3c26

Signed-off-by: Albert Teoh <albert@packsmith.io>

Default to 'server'

46989de

Signed-off-by: Albert Teoh <albert@packsmith.io>

Remove optionality in API query param fields

ccd6cc7

Signed-off-by: Albert Teoh <albert@packsmith.io>

Add behavioural tests for spanKind selector

62a71da

Signed-off-by: Albert Teoh <albert@packsmith.io>

Fix import formatting

c1ed381

Signed-off-by: Albert Teoh <albert@packsmith.io>

Fix import formatting

71c43af

Signed-off-by: Albert Teoh <albert@packsmith.io>

yarn prettier

ff1ab7b

Signed-off-by: Albert Teoh <albert@packsmith.io>

albertteoh changed the title ~~(ATM) Add span kind selector~~ (SPM) Add span kind selector Dec 3, 2023

yurishkuro approved these changes Dec 3, 2023

View reviewed changes

albertteoh merged commit 8d132ea into jaegertracing:main Dec 4, 2023
10 checks passed

albertteoh deleted the add-span-kind-selector branch December 4, 2023 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(SPM) Add span kind selector #2012

(SPM) Add span kind selector #2012

albertteoh commented Dec 2, 2023 •

edited by yurishkuro

Loading

codecov bot commented Dec 2, 2023 •

edited

Loading

albertteoh Dec 2, 2023

yurishkuro Dec 2, 2023

albertteoh Dec 3, 2023

yurishkuro Dec 2, 2023

yurishkuro Dec 2, 2023

albertteoh Dec 3, 2023

yurishkuro Dec 2, 2023

albertteoh Dec 3, 2023

yurishkuro Dec 2, 2023

albertteoh Dec 3, 2023

albertteoh commented Dec 3, 2023

yurishkuro commented Dec 3, 2023

yurishkuro Dec 3, 2023

albertteoh commented Dec 4, 2023

	<h2 className="span-kind-selector-header">Choose kind</h2>
	<h2 className="span-kind-selector-header">Span Kind</h2>

(SPM) Add span kind selector #2012

(SPM) Add span kind selector #2012

Conversation

albertteoh commented Dec 2, 2023 • edited by yurishkuro Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Dec 2, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

albertteoh commented Dec 3, 2023

yurishkuro commented Dec 3, 2023

Choose a reason for hiding this comment

albertteoh commented Dec 4, 2023

albertteoh commented Dec 2, 2023 •

edited by yurishkuro

Loading

codecov bot commented Dec 2, 2023 •

edited

Loading