-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Add inventory locator #183418
Add inventory locator #183418
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
/ci |
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.
Thanks for this PR. I haven't tested it yet.
import { Redirect, RouteComponentProps } from 'react-router-dom'; | ||
|
||
// FIXME what would be the right way to build this query string? | ||
const QUERY_STRING_TEMPLATE = |
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 { | ||
app: 'metrics', | ||
path: `/inventory?waffleFilter=${paramsWithDefaults.waffleFilter}&waffleTime=${paramsWithDefaults.waffleTime}&waffleOptions=${paramsWithDefaults.waffleOptions}&customMetrics=${paramsWithDefaults.customMetrics}&customOptions=${paramsWithDefaults.customOptions}&groupBy=${paramsWithDefaults.groupBy}&legend=${paramsWithDefaults.legend}&metric=${paramsWithDefaults.metric}&nodeType=${paramsWithDefaults.nodeType}®ion=${paramsWithDefaults.region}&sort=${paramsWithDefaults.sort}&timelineOpen=${paramsWithDefaults.timelineOpen}&view=${paramsWithDefaults.view}`, | ||
state: params.state ? params.state : {}, | ||
}; |
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.
would this work?
return { | |
app: 'metrics', | |
path: `/inventory?waffleFilter=${paramsWithDefaults.waffleFilter}&waffleTime=${paramsWithDefaults.waffleTime}&waffleOptions=${paramsWithDefaults.waffleOptions}&customMetrics=${paramsWithDefaults.customMetrics}&customOptions=${paramsWithDefaults.customOptions}&groupBy=${paramsWithDefaults.groupBy}&legend=${paramsWithDefaults.legend}&metric=${paramsWithDefaults.metric}&nodeType=${paramsWithDefaults.nodeType}®ion=${paramsWithDefaults.region}&sort=${paramsWithDefaults.sort}&timelineOpen=${paramsWithDefaults.timelineOpen}&view=${paramsWithDefaults.view}`, | |
state: params.state ? params.state : {}, | |
}; | |
const queryStringParams = queryString.stringify(paramsWithDefaults); | |
return { | |
app: 'metrics', | |
path: `/inventory?${queryStringParams}`, | |
state: params.state ? params.state : {}, | |
}; |
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 idea, thanks 👍 I started with just 3 params then decided to support more in case we need them 😅
const waffleFilter = rison.encodeUnknown(params.waffleFilter); | ||
const waffleTime = rison.encodeUnknown(params.waffleTime); | ||
const waffleOptions = rison.encodeUnknown(params.waffleOptions); | ||
const customMetrics = rison.encodeUnknown(params.customMetrics); | ||
const customOptions = rison.encodeUnknown(params.customOptions); | ||
const groupBy = rison.encodeUnknown(params.groupBy); | ||
const legend = rison.encodeUnknown(params.legend); | ||
const metric = params.metric; | ||
const nodeType = rison.encodeUnknown(params.nodeType); | ||
const region = rison.encodeUnknown(params.region); | ||
const sort = rison.encodeUnknown(params.sort); | ||
const timelineOpen = rison.encodeUnknown(params.timelineOpen); | ||
const view = rison.encodeUnknown(params.view); |
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.
const waffleFilter = rison.encodeUnknown(params.waffleFilter); | |
const waffleTime = rison.encodeUnknown(params.waffleTime); | |
const waffleOptions = rison.encodeUnknown(params.waffleOptions); | |
const customMetrics = rison.encodeUnknown(params.customMetrics); | |
const customOptions = rison.encodeUnknown(params.customOptions); | |
const groupBy = rison.encodeUnknown(params.groupBy); | |
const legend = rison.encodeUnknown(params.legend); | |
const metric = params.metric; | |
const nodeType = rison.encodeUnknown(params.nodeType); | |
const region = rison.encodeUnknown(params.region); | |
const sort = rison.encodeUnknown(params.sort); | |
const timelineOpen = rison.encodeUnknown(params.timelineOpen); | |
const view = rison.encodeUnknown(params.view); | |
const expected = Object.keys(params).reduce((acc, key) => { | |
acc[key] = key === 'metric' ? params[key] : rison.encodeUnknown(params[key]); | |
return acc; | |
}, {}); |
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.
Yeah, it makes sense, fixed! Sorry about those I was testing with different things and with/without encoding and forgot to remove and fix that.
|
||
expect(app).toBe('metrics'); | ||
expect(path).toBe( | ||
`/inventory?waffleFilter=${waffleFilter}&waffleTime=${waffleTime}&waffleOptions=${waffleOptions}&customMetrics=${customMetrics}&customOptions=${customOptions}&groupBy=${groupBy}&legend=${legend}&metric=${metric}&nodeType=${nodeType}®ion=${region}&sort=${sort}&timelineOpen=${timelineOpen}&view=${view}` |
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 think it should also work here
`/inventory?waffleFilter=${waffleFilter}&waffleTime=${waffleTime}&waffleOptions=${waffleOptions}&customMetrics=${customMetrics}&customOptions=${customOptions}&groupBy=${groupBy}&legend=${legend}&metric=${metric}&nodeType=${nodeType}®ion=${region}&sort=${sort}&timelineOpen=${timelineOpen}&view=${view}` | |
`/inventory?queryString.stringify(expected);` |
|
||
expect(app).toBe('metrics'); | ||
expect(path).toBe( | ||
`/inventory?waffleFilter=${waffleFilter}&waffleTime=${waffleTime}&waffleOptions=${waffleOptions}&customMetrics=${customMetrics}&customOptions=${customOptions}&groupBy=${groupBy}&legend=${legend}&metric=${metric}&nodeType=${nodeType}®ion=${region}&sort=${sort}&timelineOpen=${timelineOpen}&view=${view}` |
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.
same here
customMetrics: params.customMetrics || rison.encodeUnknown(''), | ||
customOptions: params.customOptions || rison.encodeUnknown(''), |
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 is needed rison.encodeUnknown('')
needed?
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.
It adds ''
(so &customMetrics=''&customOptions=''
) in the URL otherwise 🤷♀️ I think I can just leave the customMetrics: params.customMetrics
to avoid that
@crespocarlos Thank you for the review! I think I fixed the things you mentioned, thanks for the suggestions. Can you please check it again? |
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 for the changes.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Page load bundle
History
To update your PR or re-run it, just comment with: |
Relates to #176667
Summary
This PR adds an inventory locator and replaces the old redirect logic in
RedirectToInventory
Testing
To simulate the usage of
link-to
use similar params to what alerts link is using, for example:/app/metrics/link-to/inventory?customMetric=&metric=(type%3Acpu)&nodeType=pod×tamp=1715703274459
Compare the pages with the edge cluster to make sure that it looks the same as before