grafana / loki

Like Prometheus, but for logs.
https://grafana.com/loki
GNU Affero General Public License v3.0
23.96k stars 3.46k forks source link

Entry package #1417

Closed cyriltovena closed 4 years ago

cyriltovena commented 4 years ago

I was searching in our code base entry struct and find out many package has its own entry struct implementation.

I think we can create a new entry package where we would have the type share and tools for conversion and sorting.

yindia commented 4 years ago

@cyriltovena Would like to work on this. Can this be assigned to me ? I also need small brief

owen-d commented 4 years ago

Sure, I can help with context. Here's a list of files that define similar entry structs

./cmd/fluent-bit/loki_test.go
12:type entry struct {
13-     lbs  model.LabelSet
14-     line string
15-     ts   time.Time
16-}

./pkg/loghttp/query.go
80:type Entry struct {
81-     Timestamp time.Time
82-     Line      string
83-}
84-

./pkg/chunkenc/memchunk.go
137:type entry struct {
138-    t int64
139-    s string
140-}
141-

./pkg/logproto/logproto.pb.go
416:type Entry struct {
417-    Timestamp time.Time `protobuf:"bytes,1,opt,name=timestamp,proto3,stdtime" json:"ts"`
418-    Line      string    `protobuf:"bytes,2,opt,name=line,proto3" json:"line"`
419-}
420-

./pkg/loghttp/legacy/query.go
19:type Entry struct {
20-     Timestamp time.Time `json:"ts"`
21-     Line      string    `json:"line"`
22-}

./pkg/promtail/api/entry_parser_test.go
17:type Entry struct {
18-     Time   time.Time
19-     Log    string
20-     Labels model.LabelSet
21-}

./pkg/promtail/client/client.go
99:type entry struct {
100-    tenantID string
101-    labels   model.LabelSet
102-    logproto.Entry
103-}

In order to avoid redundancies, type conversions, and a generally painful programming experience when crossing the borders between different definitions, we'll probably want to centralize definitions. This may be served by a model package. Perhaps something like https://github.com/prometheus/common/blob/master/model/time.go

aartij17 commented 4 years ago

@evalsocket - Are you still working on this? If not, I would like to pick it up. Let me know!

owen-d commented 4 years ago

I’m guessing we’ll probably need a few variants, perhaps like this:

type Entry struct {
    Ts int64
    Line string
}

type Entries []Entry

type LabeledEntry struct {
    Entry
    Labels model.LabelSet
}

type LabeledEntries struct {
    Entries Entries
    Labels  model.LabelSet
}

This way we have a few structs for different use cases: 1) when we just have a log line 2) When we have a collection of log lines 3) when we have a log line with labels attached 4) when we have a collection of log lines that share labels

I'm unsure about using int64 vs time.Time for the Ts field.

cyriltovena commented 4 years ago

Package name could be model as suggested by owen

owen-d commented 4 years ago

This may be best fit for another issue, but I've experienced a lot of friction with some other type conversions. Examples include

These are just off the top of my head, but illustrate some pain pts.

I keep thinking we really need to refactor to remove type duplications everywhere: 1) Type differences confuse me b/c I need to investigate whether it’s done for a reason or is just a relic of convenience — I actually lose a lot of time this way 2) It’s super aggravating 3) We do a lot of O(n) conversions unnecessarily

/cc @slim-bean @cyriltovena

slim-bean commented 4 years ago

Closing this because it's extremely hard to get a consensus on the direction to take it, we aren't going to forget about it and i'm sure it will bet revisited some day but I don't see the value of keeping it open at the moment.