Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move filters' source of truth to the backend #148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+4,029 −3,779
Open

Move filters' source of truth to the backend #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions cmds/admin_server/server/descripe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filename typo


import (
"fmt"
"reflect"
"strings"
)

const (
INT = "int"
UINT = "uint"
STRING = "string"
TIME = "time"
ENUM = "enum"
)

type FieldMetaData struct {
Name string `json:"name"`
Type string `json:"type"`
Values []string `json:"values"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to note, in this design, the Values field only makes sense for a particular value of Type. you might wanna think about using different FieldMetaData struct variants or some other design without this possibility of invalid state (as in, having a non-nil Values when Type=="int" is invalid)

Comment on lines +9 to +20
Copy link
Member

@xaionaro xaionaro Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning

This all requires description.

}

type Description []FieldMetaData

/*
DescribeEntity returns a Description for some entity fields
to help frontend renders appropiate filters and tables.
it depends on the tags assigned to the entity fields to create the needed metadata.
it expects tags: filter, values
*/
Comment on lines +25 to +30
Copy link
Member

@xaionaro xaionaro Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking

DescribeEntity returns a Description for some entity fields to help frontend renders appropiate filters and tables.

Would be nice to have an example of such entity (which is supposed to be used here).

func DescribeEntity(entity interface{}) (Description, error) {
var describtion Description
t := reflect.TypeOf(entity)

for i := 0; i < t.NumField(); i++ {
sf := t.Field(i)
if sf.Anonymous {
return nil, fmt.Errorf("Error Anonymous Field")
}

name := sf.Tag.Get("json")
tagValue := sf.Tag.Get("filter")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you figure out these data types automatically? you already have that access.
also "filter" isnt really conducive to this usage. you might want to use a different name, if it's still necessary (as per point above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you figure out these data types automatically? you already have that access.

the tag fitler:"<type>" does not necessary match the field type, it refers to the type of frontend component that should be rendered to apply filters on that field. e.g. we could have x map[string]int `filter:"string" to search on the map keys for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should document this. it's not obvious. also not obvious how that map/string example works.

var values []string
if tagValue == ENUM {
values = strings.Split(sf.Tag.Get("values"), ",")
}
describtion = append(describtion, FieldMetaData{
Name: name,
Type: tagValue,
Values: values,
})
}

return describtion, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return describtion, nil
return description, nil

Comment on lines +31 to +54
Copy link
Member

@xaionaro xaionaro Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opinion

I understand the initial idea (of why we decided to use reflection in the first place) is that this is generic logic (not coupled with the server), thus it seems like it should be a dedicated package. The same thought from another angle: basically the whole problem with pushing the source of truth of available (for filtering) values to the backend is pretty typical; may be the solution of this specific problem should be agnostic of ConTest-specifics (including of these admin_server specifics) and thus be separated from package server.

}
30 changes: 24 additions & 6 deletions cmds/admin_server/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,15 @@ var (
DefaultDBAccessTimeout time.Duration = 10 * time.Second
)

/*
Query should have the same names as the fields in the entity that it queries(in this case Log).
To generalize filtering time fields (range filtering), it should have two fields with prefixes `start_<name>`, `end_<name>`
to make the frontend generate the query programmatically.
e.g. (Log.Date `date` -> Query.StartDate `start_date`, Query.EndDate `end_date`)
*/
type Query struct {
JobID *uint64 `form:"job_id"`
Text *string `form:"text"`
LogData *string `form:"log_data"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was wrong with text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the frontend will form the query dynamically based on the metadata that is sent from the backend. so it's easier ( for the frontend ) to make the query fields match the log struct fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i actually dont understand your answer. Are there hidden dependencies in this chain? Do you expect to have some field names matching stuff that it shouldn't? (based on your "it's easier for the frontend to make the query fields match")

LogLevel *string `form:"log_level"`
StartDate *time.Time `form:"start_date" time_format:"2006-01-02T15:04:05.000Z07:00"`
EndDate *time.Time `form:"end_date" time_format:"2006-01-02T15:04:05.000Z07:00"`
Expand All @@ -40,7 +46,7 @@ func (q *Query) ToStorageQuery() storage.Query {
}

storageQuery.JobID = q.JobID
storageQuery.Text = q.Text
storageQuery.LogData = q.LogData
storageQuery.LogLevel = q.LogLevel
storageQuery.StartDate = q.StartDate
storageQuery.EndDate = q.EndDate
Expand All @@ -57,10 +63,10 @@ func (q *Query) ToStorageQuery() storage.Query {
}

type Log struct {
JobID uint64 `json:"job_id"`
LogData string `json:"log_data"`
Date time.Time `json:"date"`
LogLevel string `json:"log_level"`
JobID uint64 `json:"job_id" filter:"uint"`
LogData string `json:"log_data" filter:"string"`
Date time.Time `json:"date" filter:"time"`
LogLevel string `json:"log_level" filter:"enum" values:"info,debug,error,fatal,panic,warning"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you make sure these hardcoded values actually match the data? what if the logged data has "INFORMATION" as level, for example; and you only give the user the option "info" as per this values tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, i will make a db distinct query to get the values for the enum filters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure you understand what the cost of that is. you may end up hammering the db with metadata related requests, combined with missing indices and you probably get a whole server crash or something.
caching would solve some things here (with write-thru), but it's probably too much to do in the remaining time span.

}

func (l *Log) ToStorageLog() storage.Log {
Expand Down Expand Up @@ -159,6 +165,17 @@ func (r *RouteHandler) status(c *gin.Context) {
c.JSON(http.StatusOK, gin.H{"status": "live"})
}

//
func (r *RouteHandler) describeLog(c *gin.Context) {
res, err := DescribeEntity(Log{})
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"status": "err", "msg": "error while getting the storage descirbtion"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c.JSON(http.StatusInternalServerError, gin.H{"status": "err", "msg": "error while getting the storage descirbtion"})
c.JSON(http.StatusInternalServerError, gin.H{"status": "err", "msg": "error while getting the storage description"})

return
}

c.JSON(http.StatusOK, res)
}

// addLogs inserts log's batches into the database
func (r *RouteHandler) addLogs(c *gin.Context) {
var logs []Log
Expand Down Expand Up @@ -277,6 +294,7 @@ func initRouter(ctx xcontext.Context, rh RouteHandler, middlewares []gin.Handler
r.GET("/log", rh.getLogs)
r.GET("/tag", rh.getTags)
r.GET("/tag/:name/jobs", rh.getJobs)
r.GET("/log-description", rh.describeLog)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably sits better under /log/metadata or similar


// serve the frontend app
r.StaticFS("/app", FS(false))
Expand Down
7,360 changes: 3,725 additions & 3,635 deletions cmds/admin_server/server/static.go
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions cmds/admin_server/storage/mongo/mongo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ func toMongoQuery(query storage.Query) bson.D {
})
}

if query.Text != nil {
if query.LogData != nil {
q = append(q, bson.E{
Key: "log_data",
Value: bson.M{
"$regex": primitive.Regex{Pattern: *query.Text, Options: "ig"},
"$regex": primitive.Regex{Pattern: *query.LogData, Options: "ig"},
},
})
}
Expand Down
2 changes: 1 addition & 1 deletion cmds/admin_server/storage/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type Log struct {
// Query defines the different options to filter with
type Query struct {
JobID *uint64
Text *string
LogData *string
LogLevel *string
StartDate *time.Time
EndDate *time.Time
Expand Down
47 changes: 22 additions & 25 deletions cmds/admin_server/ui/src/api/logs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,36 @@
import superagent from 'superagent';

// TODO: remove the hardcoded levels
// logLevels is the possible levels for logs
export const Levels = ['panic', 'fatal', 'error', 'warning', 'info', 'debug'];

// Log defines the expected log entry returned from the api
export interface Log {
job_id: number;
log_data: string;
log_level: string;
date: string;
}

// Query defines all the possible filters to form a query
export interface Query {
job_id?: number;
text?: string;
log_level?: string;
start_date?: string;
end_date?: string;
page: number;
page_size: number;
}

// Result defines the structure of the api response to a query
export interface Result {
logs: Log[] | null;
logs: any;
count: number;
page: number;
page_size: number;
}

// getLogs returns Result that contains logs fetched according to the Query
export async function getLogs(query: Query): Promise<Result> {
export async function getLogs(query: any): Promise<Result> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usage of any really isnt ok without any kind of structural context. can you write this in a different way? (page and page_size seem to be constant, so those need to exist from the start in the type).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the query will be constructed based on the meta date that is fetched from the backend. i don't know how i can modify that to not use any ?

Copy link
Member

@xaionaro xaionaro Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it should be an array of arbitrary fields+values instead of any.

[just thinking out loud] I actually like GraphQL, because everything is already invented there and we do not need to reinvent a query language and reimplement it in there. But on the other hand it is much more complicated.

let result: superagent.Response = await superagent.get('/log').query(query);

return result.body;
}

export enum FieldType {
INT = 'int',
UINT = 'uint',
STRING = 'string',
TIME = 'time',
ENUM = 'enum',
}

export interface FieldMetaData {
name: string;
type: string;
values: string[];
}

export async function getLogDescription(): Promise<FieldMetaData[]> {
let result: superagent.Response = await superagent.get('/log-description');

return result.body;
}
62 changes: 24 additions & 38 deletions cmds/admin_server/ui/src/search_logs/log_table/log_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,37 @@
import React, { useState } from 'react';
import React, { useMemo, useState } from 'react';
import { Table, Pagination, Button, useToaster, Message } from 'rsuite';
import { Column, Cell, HeaderCell } from 'rsuite-table';
import { StandardProps } from 'rsuite-table/lib/@types/common';
import { getLogs, Log, Result } from '../../api/logs';
import { TypeAttributes } from 'rsuite/esm/@types/common';
import DateCell from './date_cell/date_cell';
import { renderColumn } from './render_columns';
import {
getLogs,
getLogDescription,
Result,
FieldMetaData,
} from '../../api/logs';
import 'rsuite/dist/rsuite.min.css';
import './log_table.scss';

export interface LogTableProps extends StandardProps {
logLevels?: string;
queryText?: string;
jobID?: number;
startDate?: Date;
endDate?: Date;
columns?: FieldMetaData[];
filters?: any;
}

export default function LogTable({
logLevels,
queryText,
jobID,
startDate,
endDate,
}: LogTableProps) {
export default function LogTable({ columns, filters }: LogTableProps) {
const [loading, setLoading] = useState<boolean>(false);
const [logs, setLogs] = useState<Log[] | null>([]);
const [logs, setLogs] = useState<any[] | null>([]);
const [count, setCount] = useState<number>(0);
const [page, setPage] = useState<number>(0);
const [limit, setLimit] = useState<number>(20);

const renderedColumns = useMemo(
() =>
columns
?.sort((a, b) => (a.type.length > b.type.length ? 1 : -1))
.map((c, idx) => renderColumn(c, idx)),
[columns]
);

const toaster = useToaster();
const pageSizes = [20, 50, 100];

Expand All @@ -41,16 +44,14 @@ export default function LogTable({
);
};
const updateLogsTable = async (page: number, limit: number) => {
getLogDescription();

setLoading(true);
try {
let result: Result = await getLogs({
job_id: jobID ?? undefined,
text: queryText,
page: page,
page_size: limit,
log_level: logLevels,
start_date: startDate?.toJSON(),
end_date: endDate?.toJSON(),
...filters,
});

setLogs(result.logs);
Expand Down Expand Up @@ -83,22 +84,7 @@ export default function LogTable({
wordWrap="break-word"
rowHeight={30}
>
<Column width={80} align="center" fixed>
<HeaderCell>JobID</HeaderCell>
<Cell className="log-table__cell" dataKey="job_id" />
</Column>
<Column width={250} align="center" fixed>
<HeaderCell>Date</HeaderCell>
<DateCell className="log-table__cell" dataKey="date" />
</Column>
<Column width={80} align="center" fixed>
<HeaderCell>Level</HeaderCell>
<Cell className="log-table__cell" dataKey="log_level" />
</Column>
<Column width={600} align="left" flexGrow={1}>
<HeaderCell>Data</HeaderCell>
<Cell className="log-table__cell" dataKey="log_data" />
</Column>
{renderedColumns}
</Table>
<div>
<Pagination
Expand Down
35 changes: 35 additions & 0 deletions cmds/admin_server/ui/src/search_logs/log_table/render_columns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from 'react';
import { FieldMetaData, FieldType } from '../../api/logs';
import { Column, Cell, HeaderCell } from 'rsuite-table';
import DateCell from './date_cell/date_cell';

export function renderColumn(field: FieldMetaData, key: any) {
switch (field.type) {
case FieldType.INT:
case FieldType.UINT:
case FieldType.ENUM:
return (
<Column width={80} align="center" fixed key={key}>
<HeaderCell>{field.name}</HeaderCell>
<Cell className="log-table__cell" dataKey={field.name} />
</Column>
);
case FieldType.STRING:
return (
<Column width={600} align="left" flexGrow={1} key={key}>
<HeaderCell>{field.name}</HeaderCell>
<Cell className="log-table__cell" dataKey={field.name} />
</Column>
);
case FieldType.TIME:
return (
<Column width={250} align="center" fixed key={key}>
<HeaderCell>{field.name}</HeaderCell>
<DateCell
className="log-table__cell"
dataKey={field.name}
/>
</Column>
);
}
}
Loading