-
Notifications
You must be signed in to change notification settings - Fork 15
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
NETOBSERV-287 Aggregate stats, fix limit reach #136
Conversation
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.
Can't access to topology view anymore:
Unable to get topology
TypeError: Cannot read properties of undefined (reading 'result')
It would be great to show stats as JSON in the summary panel. Feel free to create a followup task if you prefer.
const aggQR: AggregatedQueryResponse = r.data; | ||
return { | ||
records: (aggQR.result as StreamResult[]).flatMap(r => parseStream(r)), | ||
stats: aggQR.stats | ||
}; |
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.
Need to do the same logic in getTopology
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.
return (r.data.data.result as TopologyMetrics[])
needs to be return (r.data.result as TopologyMetrics[])
.then(result => { | ||
setFlows(result.records); | ||
setStats(result.stats); | ||
}) |
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.
Also update case 'topology':
below
- Stats are now aggregated in backend (needed when multiple queries are performed) - Add a few tests - On the frontend, limit reached now comes as a stats provided from backend - Add info tooltip on limit setting, to clarify it's the limit per loki query, so we may end up with more flows than this limit
Thanks, yeah I forgot about topology on the frontend, should be fixed now. |
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, thanks @jotak !
thanks Julien |
/unhold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
backend
query, so we may end up with more flows than this limit