-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] Add sparklines to the multi-signal view table #187782
[APM] Add sparklines to the multi-signal view table #187782
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
x-pack/plugins/observability_solution/apm/server/routes/entities/services/routes.ts
Outdated
Show resolved
Hide resolved
terms: { | ||
field: LOG_LEVEL, | ||
include: ['error', 'ERROR'], | ||
}, | ||
}); | ||
|
||
type LogErrorsAggregation = ReturnType<typeof getLogErrorsAggegation>; |
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.
nice
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.
LGTM, I left a comment
(nit): Looking at the Name Column, its Left Aligned, Will it look better to have the other columns too left aligned. At the moment looking at Mixed NA and data scenarios, this UI looks a bit wierd |
@achyutjhunjhunwala All APM tables have the same alignment, it's not something introduced here - the sparklines will look bad if we change the alignment (will move to the left). I agree that we should improve the UI so we can add an issue for that, wdyt? |
getRandomSampler({ security, request, probability }), | ||
]); | ||
|
||
if (!serviceNames.length) { |
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.
We could make this validation before the API calls to save time in case serviceNames
is 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.
Good point, I missed that when I got this part from the service overview request, moved 👍
}, | ||
// only fetches detailed statistics when requestId is invalidated by main statistics api call or offset is changed | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[mainStatisticsData.requestId, services], |
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 is the dependency that causes the API to be unnecessarily called?
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.
This is similar to the request we have in the services overview (POST /internal/apm/services/detailed_statistics
) It's start
, end
, environment
, kuery
, and dataSourceOptions
in this case
'1m' | ||
); | ||
|
||
logSynthtrace.index([ |
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.
logSynthtrace.index([ | |
await logSynthtrace.index([ |
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.
Fixed, thanks ✅
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Public APIs missing exports
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
|
@achyutjhunjhunwala I added an issue to improve the table alignment: #188060 |
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.
LGTM
Closes #187567
Summary
This PR adds sparklines to the multi-signal view table
Testing
observability:apmEnableMultiSignal
in advanced settings2. Run the entities definition in the dev tools
Generate data with synthrace
node scripts/synthtrace simple_logs.ts
node scripts/synthtrace simple_trace.ts
Open services inventory