feat(detected-fields): get fields from /detected_fields API
#518
Conversation
| @@ -316,6 +327,10 @@ const emptyStateStyles = { | |||
| }), | |||
| }; | |||
|
|
|||
| function getFieldByLabel(fields: Array<SelectableValue<DetectedField>>, fieldLabel: string | undefined) { | |||
There was a problem hiding this comment.
We should really not define functions with required arguments that can be undefined. Rather, we should not call this function if fieldLabel is undefined.
Random, but, for example, this would return a field with undefined label, if both things happen at the same time. In any case, even without running conditions, an undefined required parameter is not a good funciton signature.
There was a problem hiding this comment.
True! Fun fact, and this should not be an excuse, I used the "Refactor -> Extract as function" functionality for this.
| @@ -340,28 +355,23 @@ function getStyles(theme: GrafanaTheme2) { | |||
| }; | |||
| } | |||
|
|
|||
| const avgFields = ['duration', 'count', 'total', 'bytes']; | |||
| const avgFieldTypes = ['duration']; | |||
There was a problem hiding this comment.
I've no context, why did you remove the other fields? Do we need the array?
There was a problem hiding this comment.
Good shout. On thing that changed is that we now make a decision based on the "type" the API returns, not based on the field name anymore. But I was in fact missing "bytes" here.
src/services/variables.ts
Outdated
| @@ -17,3 +17,19 @@ export const EXPLORATION_DS = { uid: VAR_DATASOURCE_EXPR }; | |||
| export const ALL_VARIABLE_VALUE = '$__all'; | |||
| export const LEVEL_VARIABLE_VALUE = 'detected_level'; | |||
| export const PATTERNS_TEXT_FILTER = 'patternsFilter'; | |||
|
|
|||
| export class DetectedField { | |||
There was a problem hiding this comment.
This should definitely not be here.
probably based on the last main merge. thank you, i'll take a look! |
| import { getSortByPreference } from 'services/store'; | ||
| import { StatusWrapper } from './StatusWrapper'; | ||
|
|
||
| export class DetectedField { |
There was a problem hiding this comment.
Apologies, I was pretty vague with my previous comment. Ideally, my suggestion would be to just move this to its own little module file. Can be under services, or feel free to introduce a models or data folder.
There was a problem hiding this comment.
Makes total sense!
|
We can "ignore" ☝️ , as it also happens in main. |
This PR uses Loki's
/detected_fieldsAPI to get fields from there. The PR also contains a lot of renaming, thus there are quite a lot of small changes.For the user, nothing really should change.
Note: This does only work with a recent Grafana version that contains grafana/grafana#89849
Fixes #245