Move filters' source of truth to the backend #148

mahmednabil109 · 2022-09-06T10:15:20Z

Signed-off-by: Mohamed Abokammer [email protected]

Goal:

to make the filters in the frontend rendered dynamically based on the backend entity (in this case
log) that we are applying the filters on, So that when any change is to be made to the backend log structure (like adding new fields to the log struct) the frontend will not need to be modified.

Frontend:

the rendered ui does not have much changes.

mimir-d

please:

add PR description of the problem you're solving (not just the title), and how you're solving it
add some screenshots since youre also modifying UI
reconsider the code design in the UI (or overall) since using any all over the place completely denies the type safety of typescript

mimir-d · 2022-09-06T16:27:08Z

cmds/admin_server/server/server.go

 type Query struct {
 	JobID     *uint64    `form:"job_id"`
-	Text      *string    `form:"text"`
+	LogData   *string    `form:"log_data"`


what was wrong with text?

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.

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")

mimir-d · 2022-09-06T16:28:47Z

cmds/admin_server/server/server.go

@@ -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)


this probably sits better under /log/metadata or similar

mimir-d · 2022-09-06T16:29:39Z

cmds/admin_server/server/descripe.go

@@ -0,0 +1,55 @@
+package server


filename typo

mimir-d · 2022-09-06T16:40:08Z

cmds/admin_server/server/descripe.go

+		}
+
+		name := sf.Tag.Get("json")
+		tagValue := sf.Tag.Get("filter")


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)

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.

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

mimir-d · 2022-09-06T16:42:26Z

cmds/admin_server/server/server.go

+	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"`


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

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

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.

mimir-d · 2022-09-06T16:42:47Z

cmds/admin_server/server/server.go

+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"})


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"})

mimir-d · 2022-09-06T16:43:17Z

cmds/admin_server/server/descripe.go

+		})
+	}
+
+	return describtion, nil


Suggested change

return describtion, nil

return description, nil

mimir-d · 2022-09-06T16:45:05Z

cmds/admin_server/server/descripe.go

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


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)

mimir-d · 2022-09-06T16:57:01Z

cmds/admin_server/ui/src/api/logs.ts

    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> {


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).

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 ?

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.

xaionaro · 2022-09-06T17:39:04Z

cmds/admin_server/server/descripe.go

+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"`


warning

This all requires description.

xaionaro · 2022-09-06T17:40:44Z

cmds/admin_server/server/descripe.go

+/*
+	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
+*/


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).

Signed-off-by: Mohamed Abokammer <[email protected]>

xaionaro · 2022-09-07T14:59:19Z

cmds/admin_server/server/descripe.go

+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")
+		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


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.

xaionaro · 2022-09-07T15:15:55Z

cmds/admin_server/ui/src/search_logs/search_logs.tsx

+    useEffect(() => {
+        (async () => {
+            try {
+                let res = await getLogDescription();
+                setDescription(res);
+            } catch (err) {
+                console.log(err);
+                showMsg('error', err?.message);
+            }
+        })();
+    }, []);


As @mimir-d already mentioned, it feels a bit dangerous. It could overload the DB. Would be nice to find some server-side solution for that (like caching DB response).

mahmednabil109 changed the title ~~Make ui filters generic~~ Move filters' SOT to the backend Sep 6, 2022

mahmednabil109 force-pushed the generic-filtering branch 3 times, most recently from ac21969 to b3db303 Compare September 6, 2022 14:44

mahmednabil109 marked this pull request as ready for review September 6, 2022 14:59

mahmednabil109 changed the title ~~Move filters' SOT to the backend~~ Move filters' source of truth to the backend Sep 6, 2022

mimir-d requested a review from xaionaro September 6, 2022 16:24

mimir-d requested changes Sep 6, 2022

View reviewed changes

xaionaro reviewed Sep 6, 2022

View reviewed changes

Make ui filters generic

aa296ee

Signed-off-by: Mohamed Abokammer <[email protected]>

mahmednabil109 force-pushed the generic-filtering branch from b3db303 to aa296ee Compare September 7, 2022 10:12

xaionaro reviewed Sep 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move filters' source of truth to the backend #148

Move filters' source of truth to the backend #148

mahmednabil109 commented Sep 6, 2022 •

edited

Loading

mimir-d left a comment

mimir-d Sep 6, 2022

mahmednabil109 Sep 7, 2022

mimir-d Sep 7, 2022

mimir-d Sep 6, 2022

mimir-d Sep 6, 2022

mimir-d Sep 6, 2022

mahmednabil109 Sep 7, 2022

mimir-d Sep 7, 2022

mimir-d Sep 6, 2022

mahmednabil109 Sep 7, 2022

mimir-d Sep 7, 2022

mimir-d Sep 6, 2022

mimir-d Sep 6, 2022

mimir-d Sep 6, 2022

mimir-d Sep 6, 2022

mahmednabil109 Sep 7, 2022

xaionaro Sep 7, 2022 •

edited

Loading

xaionaro Sep 6, 2022 •

edited

Loading

xaionaro Sep 6, 2022 •

edited

Loading

xaionaro Sep 7, 2022 •

edited

Loading

xaionaro Sep 7, 2022 •

edited

Loading

	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"})

Move filters' source of truth to the backend #148

Are you sure you want to change the base?

Move filters' source of truth to the backend #148

Conversation

mahmednabil109 commented Sep 6, 2022 • edited Loading

mimir-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xaionaro Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

xaionaro Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

xaionaro Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

xaionaro Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

xaionaro Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

mahmednabil109 commented Sep 6, 2022 •

edited

Loading

xaionaro Sep 7, 2022 •

edited

Loading

xaionaro Sep 6, 2022 •

edited

Loading

xaionaro Sep 6, 2022 •

edited

Loading

xaionaro Sep 7, 2022 •

edited

Loading

xaionaro Sep 7, 2022 •

edited

Loading