-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Explore:Traces Empty state #10724
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
Explore:Traces Empty state #10724
Conversation
Signed-off-by: Adam Tackett <[email protected]>
ℹ️ Manual Changeset Creation ReminderPlease ensure manual commit for changeset file 10724.yml under folder changelogs/fragments to complete this PR. If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link. For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool. |
Signed-off-by: Adam Tackett <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10724 +/- ##
==========================================
+ Coverage 60.34% 60.36% +0.01%
==========================================
Files 4462 4463 +1
Lines 119495 119589 +94
Branches 19748 19771 +23
==========================================
+ Hits 72109 72189 +80
- Misses 42383 42391 +8
- Partials 5003 5009 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
</div> | ||
|
||
{/* Filter badges section */} | ||
{spanFilters.length > 0 && ( |
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.
Nit: doesn't have to be handled now, but personally, I think a good way of cutting down large UI components and making sure that they are a lot more readable is to separate out React UI components by what they would be semantically, just so that I could look at something and go "oh those are the headers, that is that thing, etc". Even with functions that are put into props, putting those in functions defined up above or in util files makes this a lot more readable.
This might not make sense in all cases so take this with a grain of salt.
const presentFields: string[] = []; | ||
|
||
const spanId = extractFieldFromRowData(rowData, SPAN_ID_FIELD_PATHS); | ||
if (spanId) { |
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.
I've noticed that there is a repetitive pattern here with extract -> check -> push. We could try and make this a bit more maintainable and readable.
(Completely optional suggestion):
We have an array of each of these fields we are checking for - this array could contain the name, the path we expect to find it at (to handle nested values), anything else needed.
Then, we have this function work to perform validation after on that array.
A potential downside of doing my suggestion is that each field we check for is abstract enough that all of the various options inside of the array that we put down could grow to be larger than just doing this manually (which could be why you've done it this way). I haven't fully checked but take a look and see if this could help.
Description
Explore:Traces Empty state
Add error handling for missing fields by informing user.
Disabled the span-id re-direction if it would lead to a broken page.
Issues Resolved
Screenshot
SpanId disabled:

Fly-out with info:

Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration