Make changes to allow for /api/quality-metrics to work #2641

ADI-ROXX · 2025-01-28T15:56:22Z

Which problem is this PR solving?

Description of the changes

introduce config option for the API endpoint and change it to `/api/quality-metrics' as implemented in Create a /quality-metrics endpoint jaeger#6608

How was this change tested?

manually

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: npm run lint and npm run test

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX · 2025-01-28T15:56:55Z

@yurishkuro Some tests are failing. I have to make changes in the .test.js files

yurishkuro · 2025-01-30T22:37:20Z

lgtm. Is it correct that this is a breaking change (different data shape is expected)? Please document it in the description.

yurishkuro · 2025-01-30T22:37:50Z

can you get the CI to green? Both linter and unit tests are failing.

ADI-ROXX · 2025-02-03T05:09:30Z

Is it correct that this is a breaking change (different data shape is expected)?

I didn't understand. Can you elaborate it a bit? What data shape is actually expected?

yurishkuro · 2025-02-03T12:11:38Z

You are now parsing data in a different shape than before (the "data" sub-structure in json)

ADI-ROXX · 2025-02-03T15:59:49Z

You are now parsing data in a different shape than before (the "data" sub-structure in json)

Are you referring to this:

yurishkuro · 2025-02-03T16:06:29Z

yes

Signed-off-by: cs-308-2023 <[email protected]>

codecov · 2025-02-12T14:40:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.62%. Comparing base (625a57a) to head (c2a896d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2641   +/-   ##
=======================================
  Coverage   96.62%   96.62%           
=======================================
  Files         256      256           
  Lines        7732     7733    +1     
  Branches     2018     2018           
=======================================
+ Hits         7471     7472    +1     
  Misses        261      261

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

packages/jaeger-ui/src/components/QualityMetrics/__snapshots__/index.test.js.snap

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX · 2025-02-12T18:14:35Z

Please document it in the description.

Added in the description of the PR

## Which problem is this PR solving? - Resolves #6607 - Related to jaegertracing/jaeger-ui#2640 - Related to the PR: jaegertracing/jaeger-ui#2641 ## Description of the changes - ## How was this change tested? - ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: cs-308-2023 <[email protected]>

yurishkuro · 2025-02-12T19:57:21Z

packages/jaeger-ui/src/components/QualityMetrics/index.tsx

@@ -92,9 +92,10 @@ export class UnconnectedQualityMetrics extends React.PureComponent<TProps, TStat
    this.setState({ loading: true });

    JaegerAPI.fetchQualityMetrics(service, lookback)
-      .then((qualityMetrics: TQualityMetrics) => {
-        this.setState({ qualityMetrics, loading: false });
+      .then((qualityMetrics: { data: TQualityMetrics }) => {


I don't understand why we need to be making a breaking change to the data format? The current code does not expect data: element. The backend code does not require it either.

Signed-off-by: Yuri Shkuro <[email protected]>

## Which problem is this PR solving? - Follow up to #6608 - Resolves #6607 ## Description of the changes - Remove intermediate `data:` element from the response - Match response to what the UI expects - Fix unit test that wasn't testing the output ## How was this change tested? - unit tests - manual test with jaegertracing/jaeger-ui#2641 --------- Signed-off-by: Yuri Shkuro <[email protected]>

Modified url and the struct

61e5f16

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX requested a review from a team as a code owner January 28, 2025 15:56

ADI-ROXX requested review from jkowall and removed request for a team January 28, 2025 15:56

ADI-ROXX added 2 commits February 4, 2025 17:01

Lint and some tests

397ab48

Signed-off-by: cs-308-2023 <[email protected]>

All tests solved

df12de0

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX force-pushed the QualityMetrics branch from 370cf81 to adefd46 Compare February 12, 2025 14:33

mid

e757854

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX force-pushed the QualityMetrics branch from adefd46 to e757854 Compare February 12, 2025 14:36

yurishkuro reviewed Feb 12, 2025

View reviewed changes

packages/jaeger-ui/src/components/QualityMetrics/__snapshots__/index.test.js.snap Outdated Show resolved Hide resolved

Snapshots

34c3d97

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX mentioned this pull request Feb 12, 2025

Create a /quality-metrics endpoint jaegertracing/jaeger#6608

Merged

4 tasks

Lint

31a9d39

Signed-off-by: cs-308-2023 <[email protected]>

ADI-ROXX force-pushed the QualityMetrics branch from d456f23 to 31a9d39 Compare February 12, 2025 17:05

yurishkuro reviewed Feb 12, 2025

View reviewed changes

yurishkuro and others added 3 commits February 15, 2025 09:10

Merge branch 'main' into QualityMetrics

9060578

fix

8a114fa

Signed-off-by: Yuri Shkuro <[email protected]>

add config

dff8a6a

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Feb 15, 2025

fix

c2a896d

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro enabled auto-merge February 15, 2025 13:33

yurishkuro approved these changes Feb 15, 2025

View reviewed changes

yurishkuro added this pull request to the merge queue Feb 15, 2025

yurishkuro mentioned this pull request Feb 15, 2025

Fix /qualitymetrics to return data in expected format jaegertracing/jaeger#6733

Merged

Merged via the queue into jaegertracing:main with commit 655f2b7 Feb 15, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes to allow for /api/quality-metrics to work #2641

Make changes to allow for /api/quality-metrics to work #2641

ADI-ROXX commented Jan 28, 2025 •

edited by yurishkuro

Loading

ADI-ROXX commented Jan 28, 2025

yurishkuro commented Jan 30, 2025

yurishkuro commented Jan 30, 2025

ADI-ROXX commented Feb 3, 2025

yurishkuro commented Feb 3, 2025

ADI-ROXX commented Feb 3, 2025

yurishkuro commented Feb 3, 2025

codecov bot commented Feb 12, 2025 •

edited

Loading

ADI-ROXX commented Feb 12, 2025

yurishkuro Feb 12, 2025

Make changes to allow for /api/quality-metrics to work #2641

Make changes to allow for /api/quality-metrics to work #2641

Conversation

ADI-ROXX commented Jan 28, 2025 • edited by yurishkuro Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

ADI-ROXX commented Jan 28, 2025

yurishkuro commented Jan 30, 2025

yurishkuro commented Jan 30, 2025

ADI-ROXX commented Feb 3, 2025

yurishkuro commented Feb 3, 2025

ADI-ROXX commented Feb 3, 2025

yurishkuro commented Feb 3, 2025

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

ADI-ROXX commented Feb 12, 2025

yurishkuro Feb 12, 2025

Choose a reason for hiding this comment

ADI-ROXX commented Jan 28, 2025 •

edited by yurishkuro

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading