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

Create a /quality-metrics endpoint #6608

Merged
merged 17 commits into from
Feb 12, 2025
Merged

Conversation

ADI-ROXX
Copy link
Contributor

@ADI-ROXX ADI-ROXX commented Jan 25, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@ADI-ROXX ADI-ROXX requested a review from a team as a code owner January 25, 2025 16:34
@ADI-ROXX ADI-ROXX requested a review from pavolloffay January 25, 2025 16:34
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Copy link

codecov bot commented Jan 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.04%. Comparing base (8bf69c7) to head (6f434d4).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6608      +/-   ##
==========================================
+ Coverage   95.99%   96.04%   +0.04%     
==========================================
  Files         363      364       +1     
  Lines       20568    20699     +131     
==========================================
+ Hits        19745    19881     +136     
+ Misses        628      624       -4     
+ Partials      195      194       -1     
Flag Coverage Δ
badger_v1 9.80% <ø> (-0.03%) ⬇️
badger_v2 1.81% <ø> (-0.01%) ⬇️
cassandra-4.x-v1-manual 14.76% <ø> (-0.04%) ⬇️
cassandra-4.x-v2-auto 1.80% <ø> (-0.01%) ⬇️
cassandra-4.x-v2-manual 1.80% <ø> (-0.01%) ⬇️
cassandra-5.x-v1-manual 14.76% <ø> (-0.04%) ⬇️
cassandra-5.x-v2-auto 1.80% <ø> (-0.01%) ⬇️
cassandra-5.x-v2-manual 1.80% <ø> (-0.01%) ⬇️
elasticsearch-6.x-v1 19.08% <ø> (-0.05%) ⬇️
elasticsearch-7.x-v1 19.16% <ø> (-0.05%) ⬇️
elasticsearch-8.x-v1 19.33% <ø> (-0.05%) ⬇️
elasticsearch-8.x-v2 1.81% <ø> (-0.01%) ⬇️
grpc_v1 10.77% <ø> (-0.03%) ⬇️
grpc_v2 7.78% <ø> (-0.03%) ⬇️
kafka-3.x-v1 10.09% <ø> (-0.03%) ⬇️
kafka-3.x-v2 1.81% <ø> (-0.01%) ⬇️
memory_v2 1.81% <ø> (-0.01%) ⬇️
opensearch-1.x-v1 19.21% <ø> (-0.05%) ⬇️
opensearch-2.x-v1 19.21% <ø> (-0.05%) ⬇️
opensearch-2.x-v2 1.81% <ø> (-0.01%) ⬇️
tailsampling-processor 0.48% <ø> (-0.01%) ⬇️
unittests 94.93% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

include a screenshot demonstrating that this works in the UI

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@yaten2302
Copy link

Hey @ADI-ROXX , I was working on this PR: Migrate DetailsCard from class based to function based component, but there's a problem that before migrating, the /quality-metrics endpoint must be present in order to show the correct results.
I see that you're working on this issue, may we collaborate on this, if it's fine with you?
Thanks

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Feb 3, 2025

Hey @ADI-ROXX , I was working on this PR: Migrate DetailsCard from class based to function based component, but there's a problem that before migrating, the /quality-metrics endpoint must be present in order to show the correct results. I see that you're working on this issue, may we collaborate on this, if it's fine with you? Thanks

Hi @yaten2302. I don't think that this PR has much change left. However, if I feel difficulty in something, I will let you know.

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Feb 8, 2025

@yurishkuro Do you have any idea for any test in mind for data.go? I don't have any idea for the same.

ADI-ROXX and others added 3 commits February 10, 2025 11:22
@ADI-ROXX
Copy link
Contributor Author

@yurishkuro Can you review it once? I don't think that any more changes are left for this PR.

package qualitymetrics

// TQualityMetrics represents the entire quality metrics response.
type TQualityMetrics struct {
Copy link
Member

Choose a reason for hiding this comment

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

why T?

Copy link
Member

Choose a reason for hiding this comment

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

what's the answer to the question?

Copy link
Member

Choose a reason for hiding this comment

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

general advice - it's a poor choice to resolve comments without actually responding or fixing the code.

@yurishkuro
Copy link
Member

Minor nits, otherwise looks great. Nice to have a proper JSON data model documented in code.

@ADI-ROXX
Copy link
Contributor Author

I will commit the changes soon. I am getting a few errors in the ui(probably local issues since I didn't change anything). I will push the final changes after checking the ui one last time.

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@ADI-ROXX
Copy link
Contributor Author

@yurishkuro I have made all the minor changes. Can you look once? Thank you.

Also, I have to ask a general question:
Is there any GUI tool that you use for testing on local for various things like coverage, tests, and linting, or do you use make cover, test, and lint in the terminal?

@yurishkuro
Copy link
Member

for Go coverage I use VSCode's built-in package coverage command

image

@ADI-ROXX
Copy link
Contributor Author

Ok. I'll try to integrate that in my workspace.

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@yurishkuro
Copy link
Member

Where is the pr for ui changes? I only see an issue linked

@ADI-ROXX
Copy link
Contributor Author

Where is the pr for ui changes?

Fixed

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>

func TestGetSampleData(t *testing.T) {
data := GetSampleData()
assert.NotEmpty(t, data.TraceQualityDocumentationLink, "TraceQualityDocumentationLink should not be empty")
Copy link
Member

Choose a reason for hiding this comment

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

tip: you don't need these messages in asserts, the assert itself already provides very readable error message with line number

Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@yurishkuro yurishkuro merged commit 42cd2b9 into jaegertracing:main Feb 12, 2025
55 checks passed
github-merge-queue bot pushed a commit to jaegertracing/jaeger-ui that referenced this pull request Feb 15, 2025
## Which problem is this PR solving?
- Resolves #2640 
- jaegertracing/jaeger#6607

## Description of the changes
- introduce config option for the API endpoint and change it to
`/api/quality-metrics' as implemented in
jaegertracing/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 <adityaruhela2003@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2025
## 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 <github@ysh.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create dummy /quality-metrics endpoint
3 participants