-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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>
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 |
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
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>
@yurishkuro Do you have any idea for any test in mind for data.go? I don't have any idea for the same. |
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
@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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why T
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Minor nits, otherwise looks great. Nice to have a proper JSON data model documented in code. |
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>
@yurishkuro I have made all the minor changes. Can you look once? Thank you. Also, I have to ask a general question: |
Ok. I'll try to integrate that in my workspace. |
Where is the pr for ui changes? I only see an issue linked |
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") |
There was a problem hiding this comment.
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>
## 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>
## 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>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test