feat(insights): add eap to http landing page #81753
Conversation
github-actions
bot
added
the
Scope: Frontend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #81753 +/- ##
==========================================
- Coverage 80.44% 80.44% -0.01%
==========================================
Files 7250 7250
Lines 322048 322043 -5
Branches 20852 20851 -1
==========================================
- Hits 259082 259077 -5
Misses 62572 62572
Partials 394 394 |
Bundle ReportChanges will increase total bundle size by 18.86kB (0.06%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
| @@ -76,6 +81,8 @@ const useDiscover = <T extends Extract<keyof ResponseType, string>[], ResponseTy | |||
| dataset: DiscoverDatasets, | |||
| referrer: string | |||
| ) => { | |||
| const {useRpc: useRpcFromQueryParam} = useEapOptions(); | |||
There was a problem hiding this comment.
Why the rename to useRpcFromQueryParam?
There was a problem hiding this comment.
EAP should be in all caps everywhere 🙏🏻 Same with RPC
| /** | ||
| * This selector allows us to with and without EAP enabled | ||
| * @returns | ||
| */ |
There was a problem hiding this comment.
This comment is ... incomplete?
| const options: Options = [ | ||
| { | ||
| value: 'noEap', | ||
| label: "Don't use EAP", | ||
| }, | ||
| { | ||
| value: 'useEap', | ||
| label: 'Use EAP', | ||
| }, | ||
| { | ||
| value: 'useEapRpc', | ||
| label: 'Use RPC EAP', | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
These can be moved out of the component since they're static
| if (hasEAPFeature) { | ||
| return ( | ||
| <CompactSelect | ||
| options={options} | ||
| value={value} | ||
| onChange={(selectedOption: SelectOption<EapOption>) => | ||
| navigate({ | ||
| ...location, | ||
| query: { | ||
| ...location.query, | ||
| eap_option: selectedOption.value, | ||
| }, | ||
| }) | ||
| } | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
You can avoid this by returning undefined as soon as the feature is available at the top of the component
There was a problem hiding this comment.
Also, isn't this redundant? useEapOptions already checks the feature flag
| default: | ||
| return {useEap: true, useRpc: false}; |
There was a problem hiding this comment.
default case turning on EAP seems kind of aggressive, since this is feature flagged
| const domainsListResponse = useEap ? eapDomainListResponse : metricsDomainsListResponse; | ||
| const { | ||
| isPending: isDurationDataLoading, | ||
| data: durationData, | ||
| error: durationError, | ||
| } = useEap ? eapDurationDataResponse : durationDataResponse; | ||
| const { | ||
| isPending: isThroughputDataLoading, | ||
| data: throughputData, | ||
| error: throughputError, | ||
| } = useEap ? eapThroughputDataResponse : throughtputDataResponse; | ||
| const { | ||
| isPending: isResponseCodeDataLoading, | ||
| data: responseCodeData, | ||
| error: responseCodeError, | ||
| } = useEap ? eapResponseRateResponse : responseRateResponse; | ||
|
|
There was a problem hiding this comment.
This is a lot of duplication 🤔 this if every single page is going to require this much work, that won't look great in the code. Is it possible to tuck this away inside a hook?
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Work for #81750
This PR adds the following to the http landing page