Add Loki log sending #5

Merged
fyrchik merged 1 commit from achuprov/frostfs-observability:loki into master 2023-11-02 07:44:24 +00:00
Member

Add package for sending logs to Loki

Related to:
TrueCloudLab/frostfs-dev-env#57
TrueCloudLab/frostfs-node#740

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Add package for sending logs to Loki Related to: https://git.frostfs.info/TrueCloudLab/frostfs-dev-env/pulls/57 https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/740 Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov force-pushed loki from 642aeb169b to 87129d7d29 2023-10-14 18:22:07 +00:00 Compare
achuprov force-pushed loki from 87129d7d29 to 04238a7ee2 2023-10-14 21:41:20 +00:00 Compare
fyrchik changed title from [#5] logs:Add Loki to [#5] logs: Add Loki 2023-10-16 09:48:19 +00:00
achuprov force-pushed loki from 04238a7ee2 to 8cee7e75c1 2023-10-19 07:33:10 +00:00 Compare
achuprov force-pushed loki from 8cee7e75c1 to f3480b9511 2023-10-19 07:35:37 +00:00 Compare
achuprov force-pushed loki from f3480b9511 to e0b6c97654 2023-10-23 08:12:22 +00:00 Compare
achuprov force-pushed loki from e0b6c97654 to eac876b88a 2023-10-25 17:39:41 +00:00 Compare
achuprov changed title from [#5] logs: Add Loki to Add Loki log sending package 2023-10-25 17:48:49 +00:00
achuprov changed title from Add Loki log sending package to Add Loki log sending 2023-10-25 17:50:33 +00:00
achuprov requested review from storage-core-committers 2023-10-25 17:50:47 +00:00
achuprov requested review from storage-core-developers 2023-10-25 17:50:48 +00:00
dstepanov-yadro reviewed 2023-10-26 15:50:21 +00:00
loki/log.go Outdated
@ -0,0 +92,4 @@
}
}
func processMissedMessages(batch *[]*logEntry) {

*[]*logEntry - wow! Could it be simplified?
You can pass slice by value, since slice contains only two int's and pointer. Also to append slice using append - style is straightforward.

`*[]*logEntry` - wow! Could it be simplified? You can pass slice by value, since slice contains only two int's and pointer. Also to append slice using `append` - style is straightforward.
Author
Member

fixed

fixed
achuprov force-pushed loki from eac876b88a to a84d2e3af0 2023-10-27 13:49:04 +00:00 Compare
dstepanov-yadro requested changes 2023-10-27 14:36:19 +00:00
loki/log.go Outdated
@ -0,0 +62,4 @@
batch = append(batch, entry)
}
batch = processMissedMessages(batch)
processBatch(batch)

Now looks like batch reset misses: after processBatch batch still contains old records.

Now looks like batch reset misses: after `processBatch` `batch` still contains old records.
Author
Member

This isn't an issue because after this, we end the function and the batch will no longer exist

This isn't an issue because after this, we end the function and the `batch` will no longer exist
dstepanov-yadro marked this conversation as resolved
fyrchik reviewed 2023-10-27 14:57:32 +00:00
@ -0,0 +16,4 @@
encoder zapcore.Encoder
}
func New(original zapcore.Core, call func(msg string, logTime time.Time) error, enabledCall func() bool) *LokiCore {
Owner

What is original core here? We can unite anything with 87577d85d5/zapcore/tee.go (L37C18-L37C18) , so 2 cores are possible

What is original core here? We can unite anything with https://github.com/uber-go/zap/blob/87577d85d58b6d92d0158967df29303d04d30e36/zapcore/tee.go#L37C18-L37C18 , so 2 cores are possible
Author
Member

This is any core that implements the Core interface. If you want to pass two cores, you can merge them using zapcore.NewTee. In this case, the lowest logging level of the two cores will be used.

This is any core that implements the `Core` interface. If you want to pass two cores, you can merge them using `zapcore.NewTee`. In this case, the lowest logging level of the two cores will be used.
fyrchik reviewed 2023-10-30 08:28:50 +00:00
loki/log.go Outdated
@ -0,0 +7,4 @@
// If the client is disabled, it does nothing.
// If the entries channel is full, the message is discarded.
func Send(msg string, timestamp time.Time) error {
Owner

We have a function here and it's only purpose is to be provided as a callback to lokicore.
Why not replace global client with a client initialized inside lokicore.New?

We have a function here and it's only purpose is to be provided as a callback to lokicore. Why not replace global client with a client initialized inside `lokicore.New`?
fyrchik marked this conversation as resolved
loki/log.go Outdated
@ -0,0 +88,4 @@
func processBatch(batch []*logEntry) []*logEntry {
if len(batch) > 0 {
sendLogs(batch)
batch = batch[:0]
Owner

Why is is []*logEntry and not []logEntry? For a slice of pointers the memory is leaking a bit after batch = batch[:0]

Why is is `[]*logEntry` and not `[]logEntry`? For a slice of pointers the memory is leaking a bit after `batch = batch[:0]`
fyrchik marked this conversation as resolved
loki/send.go Outdated
@ -0,0 +1,67 @@
package loki
Owner

IMO loki is too specific to be on the toplevel in this repo. Can we move it to logging/lokicore/loki (or lokicore/internal/loki?

IMO `loki` is too specific to be on the toplevel in this repo. Can we move it to `logging/lokicore/loki` (or `lokicore/internal/loki`?
fyrchik marked this conversation as resolved
loki/setup.go Outdated
@ -0,0 +7,4 @@
"time"
)
const LogEntriesChanSize = 5000
Owner

Is this useful for a client of the library?

Is this useful for a client of the library?
Author
Member

I don't think it makes sense to adjust this parameter since it's set to an optimal value

I don't think it makes sense to adjust this parameter since it's set to an optimal value
Owner

Then why is it exported?

Then why is it exported?
Author
Member

fix

fix
fyrchik marked this conversation as resolved
loki/setup.go Outdated
@ -0,0 +8,4 @@
)
const LogEntriesChanSize = 5000
const Path string = "/api/prom/push"
Owner

Can this path be configurable from the loki side? normalizeUrl is broken if so.

Can this path be configurable from the loki side? `normalizeUrl` is broken if so.
fyrchik marked this conversation as resolved
achuprov force-pushed loki from a84d2e3af0 to fcf7714928 2023-11-01 08:04:04 +00:00 Compare
achuprov force-pushed loki from fcf7714928 to 5a9e3ce812 2023-11-01 08:06:17 +00:00 Compare
Author
Member

@fyrchik fixed

@fyrchik fixed
achuprov force-pushed loki from 5a9e3ce812 to 658a4f3ed8 2023-11-01 10:33:46 +00:00 Compare
fyrchik approved these changes 2023-11-01 10:45:54 +00:00
@ -0,0 +5,4 @@
"time"
)
// If the client is disabled, it does nothing.
Owner

It makes no sense to create a client which is disabled, right? Why are there IsEnabled checks?

It makes no sense to create a client which is disabled, right? Why are there `IsEnabled` checks?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
achuprov force-pushed loki from 658a4f3ed8 to b3ad3335ff 2023-11-01 11:17:50 +00:00 Compare
dstepanov-yadro approved these changes 2023-11-02 07:11:13 +00:00
fyrchik reviewed 2023-11-02 07:40:08 +00:00
@ -0,0 +43,4 @@
// E.g. localhost:3100/api/prom/push.
Endpoint string
Labels map[string]string
//Maximum message buffering time.
Owner

Space after the //.

Space after the `//`.
fyrchik approved these changes 2023-11-02 07:40:42 +00:00
fyrchik merged commit b3ad3335ff into master 2023-11-02 07:44:24 +00:00
fyrchik referenced this pull request from a commit 2023-11-02 07:44:25 +00:00
Sign in to join this conversation.
No description provided.