Loki integration #2 #2306

lperdereau · 2023-06-24T18:13:50Z

Refer to : Loki integration #1574

Code updated
All old tests pass
Work with latest Loki version

github-actions · 2023-06-24T18:14:04Z

@lperdereau: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.

/kind feature
/kind enhancement
/kind fix
/kind chore
/kind dependencies

Details

I am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository.

github-actions · 2023-06-24T18:14:06Z

@lperdereau: There are no area labels on this PR. You can add as many areas as you see fit.

/area agent
/area local-api
/area cscli
/area security
/area configuration

Details

I am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository.

lperdereau · 2023-06-24T18:31:17Z

/kind feature
/area agent

codecov · 2023-07-03T20:41:02Z

Codecov Report

Attention: 195 lines in your changes are missing coverage. Please review.

Comparison is base (947b247) 56.87% compared to head (695a104) 56.97%.

Files	Patch %	Lines
...on/modules/loki/internal/lokiclient/loki_client.go	51.62%	93 Missing and 11 partials ⚠️
pkg/acquisition/modules/loki/loki.go	72.06%	56 Missing and 13 partials ⚠️
pkg/acquisition/modules/loki/entry.go	0.00%	13 Missing ⚠️
...uisition/modules/loki/internal/lokiclient/types.go	53.84%	4 Missing and 2 partials ⚠️
pkg/acquisition/modules/loki/timestamp.go	82.35%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2306      +/-   ##
==========================================
+ Coverage   56.87%   56.97%   +0.09%     
==========================================
  Files         190      195       +5     
  Lines       26169    26675     +506     
==========================================
+ Hits        14884    15197     +313     
- Misses       9729     9897     +168     
- Partials     1556     1581      +25

Flag	Coverage Δ
bats	`37.78% <0.00%> (-0.77%)`	⬇️
unit-linux	`55.45% <61.46%> (+0.23%)`	⬆️
unit-windows	`51.80% <46.40%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mmoscher · 2023-08-11T20:51:57Z

Interesting! Any news on this? What's missing and/or where is help needed?

lperdereau · 2023-08-11T20:55:19Z

Interesting! Any news on this? What's missing and/or where is help needed?

@mmoscher no news, I'm trying to keep working on it if you want to contribute we can try to list what's missing.
This is an existing branch that I've updated and increased testing by adding Loki to docker compose tests.

pschiffe · 2023-08-28T13:15:51Z

Looking forward to this feature, hopefully you'll be able to finish it 🤞

lperdereau · 2023-09-22T16:10:05Z

I hope this feature can be review by a maintainer. Just few comment about the feature and keys to complete this PR.

lperdereau · 2023-10-02T14:12:53Z

Thank you @LaurenceJJones for CI pipeline.
I've just patch the build error caused by :

package loki_test
import "github.com/crowdsecurity/go-cs-lib/pkg/cstest"

Updated into:

package loki_test
import "github.com/crowdsecurity/go-cs-lib/cstest"

buixor · 2023-10-03T08:20:32Z

Hey,

Sorry for the delay, having a look at it now and in the coming days :)
Thanks a lot,

buixor

Hello,

Here goes a quick review. It looks overall good to me; I have a few suggestions. Please let me know what you think. If we agree, it can go in the next release.

Thanks for your contribution 👍

Self note : When creating the associated documentation, we need to make it loud & clear that the user can "miss" some log lines depending on his config and the volume of logs he wants to ingest.

pkg/acquisition/modules/loki/loki.go

buixor · 2023-10-03T09:50:44Z

pkg/acquisition/modules/loki/internal/lokiclient/loki_client.go

+}
+
+func (lc *LokiClient) tailLogs(conn *websocket.Conn, c chan *LokiResponse, ctx context.Context) error {
+	tick := time.NewTicker(100 * time.Millisecond)


If we don't read "fast enough" from loki, we will lose messages (as far as I understand). While we can track it with the metrics and log messages, having a dynamic ticker might help. I suggest the following approach:

we can start with a 50ms ticker

if we don't get a message when reading, we can increase the ticker duration (ie. double it, with 100ms being the upper bound)

on the other hand, if we miss messages, we lower the ticker duration (ie. divide it by two, and go as low as 5ms)

I'm not sure about the upper/lower bound we should pick with the ticker as I don't have XP with Loki, but I hope you get the idea 😄

I don't have any information about this choice made by the previous developer.
I thought it might be a websocket connection keepalive.

I will say: If we don't read "fast enough" from loki, we will lose connection.

I did a search on grafana which allows the user to stream Loki's logs live (feature documentation).
Here the codebase grafana/grafana

We find the same idea

ticker := time.NewTicker(time.Second * 60) //.Step) defer ticker.Stop() for { select { case <-done: logger.Info("Socket done") return nil case <-ctx.Done(): logger.Info("Stop streaming (context canceled)") return nil case t := <-ticker.C: count++ logger.Error("Loki websocket ping?", "time", t, "count", count) } }

I agree with we can have a dynamic ticker.

I'm not sure about this ticker, but I still think it's a connection keeper.

I dig into grafana code base, and I understand that they have an UI who allow stream aquisition for user via a proxy API who tail loki logs.
This ticker and and the above code (in grafana repository) is used to keep connection between user's browser and Grafana Proxy API.
In this way we don't need this for our usage. I simplified the implementation.

pkg/acquisition/modules/loki/internal/lokiclient/loki_client.go

lperdereau

Thank you @buixor for the review and comment.
It's really good to see activity on this 😃

pkg/acquisition/modules/loki/loki.go

pkg/acquisition/modules/loki/internal/lokiclient/loki_client.go

lperdereau · 2023-10-03T20:34:36Z

pkg/acquisition/modules/loki/internal/lokiclient/loki_client.go

+}
+
+func (lc *LokiClient) tailLogs(conn *websocket.Conn, c chan *LokiResponse, ctx context.Context) error {
+	tick := time.NewTicker(100 * time.Millisecond)


I don't have any information about this choice made by the previous developer.
I thought it might be a websocket connection keepalive.

I will say: If we don't read "fast enough" from loki, we will lose connection.

I did a search on grafana which allows the user to stream Loki's logs live (feature documentation).
Here the codebase grafana/grafana

We find the same idea

ticker := time.NewTicker(time.Second * 60) //.Step) defer ticker.Stop() for { select { case <-done: logger.Info("Socket done") return nil case <-ctx.Done(): logger.Info("Stop streaming (context canceled)") return nil case t := <-ticker.C: count++ logger.Error("Loki websocket ping?", "time", t, "count", count) } }

I agree with we can have a dynamic ticker.

I'm not sure about this ticker, but I still think it's a connection keeper.

buixor · 2023-10-16T16:08:35Z

Hey thanks,

Having a look ! Hopefully this time I won't keep you waiting ;)

buixor · 2023-10-17T09:06:43Z

Hello @lperdereau !

I made a few minor changes in the code (bubbling on errors, not capitalizing errors and a few logging clarifications).
However during our internal testing we realized that the tail approach can lead to some missed messages. An easy way to reproduce it is like using ab (concurrency of 10 with few thousand messages: ab -n 5000 -c 10 localhost/ ) on a web-server plugged to loki: Loki tail is going to miss a significant part of the messages.

While investigating, we found out that, according to this issue, The tail endpoint is not built with the intention of providing all logs for a stream 😞

We are experimenting with the query approach that another guy recommends. I will let you know if the PoC is conclusive.

Stay tuned, you can drop by on discord if you want to chat about it!

buixor · 2023-10-17T14:53:20Z

Added a PR on your PR : lperdereau#1

lperdereau · 2023-10-18T09:18:18Z

Hello @lperdereau !

I made a few minor changes in the code (bubbling on errors, not capitalizing errors and a few logging clarifications). However during our internal testing we realized that the tail approach can lead to some missed messages. An easy way to reproduce it is like using ab (concurrency of 10 with few thousand messages: ab -n 5000 -c 10 localhost/ ) on a web-server plugged to loki: Loki tail is going to miss a significant part of the messages.

While investigating, we found out that, according to this issue, The tail endpoint is not built with the intention of providing all logs for a stream 😞

We are experimenting with the query approach that another guy recommends. I will let you know if the PoC is conclusive.

Stay tuned, you can drop by on discord if you want to chat about it!

Oh right, I'll check that out, i didn't notice this, no problem for a query acquisition mode.

…quisition failure can bubble up

buixor

Thanks a lot @lperdereau for the contribution. Tests and code is fine for me. It shall ship in 1.5.6 🥳

github-actions bot added the needs/kind label Jun 24, 2023

github-actions bot added the needs/area label Jun 24, 2023

github-actions bot added kind/feature area/agent and removed needs/kind needs/area labels Jun 24, 2023

lperdereau marked this pull request as draft June 25, 2023 18:47

lperdereau marked this pull request as ready for review June 25, 2023 18:58

lperdereau force-pushed the loki branch from e6c524a to d6988c2 Compare June 26, 2023 14:17

lperdereau force-pushed the loki branch from 4f46ea4 to 9a9c8d8 Compare July 3, 2023 20:12

lperdereau force-pushed the loki branch 2 times, most recently from 7eb48f1 to c6a2c27 Compare July 3, 2023 20:48

lperdereau force-pushed the loki branch from e1d7fed to a8358b8 Compare September 22, 2023 16:04

lperdereau force-pushed the loki branch from a8358b8 to 6fc6421 Compare October 2, 2023 13:14

buixor requested changes Oct 3, 2023

View reviewed changes

buixor mentioned this pull request Oct 3, 2023

Loki integration #1574

Closed

7 tasks

lperdereau commented Oct 3, 2023

View reviewed changes

lperdereau force-pushed the loki branch 3 times, most recently from c6d1e1a to 2ad0b79 Compare October 9, 2023 16:33

lperdereau added 10 commits October 15, 2023 21:49

fix: Update auth yaml config

810f525

fix: Add https to dsn

2017e6b

feat: Add to detect test

9520670

fix: Add loki readiness probe to CI

5acf6a8

fix: Path health command for loki

8d05565

fix: Patch loki acquisition

fc33f8d

fix: Update go-cs-lib/cstest

a3e4199

fix: Update go.mod and go.sum

efc6213

feat: Add delay for on tail request

e750412

feat: Refactor Tail and add metrics

a3eff54

lperdereau force-pushed the loki branch from e269e7b to a3eff54 Compare October 15, 2023 19:49

Merge branch 'master' into loki

1a8c127

buixor added 3 commits October 17, 2023 09:47

improve bubble up errors and fix a few logging messages

755ca02

improve bubble up errors and fix a few logging messages

9341d3f

Merge branch 'master' into loki

f39c61e

Merge branch 'master' into loki

a54090b

blotus and others added 5 commits October 18, 2023 11:26

use query_range instead of tail

5029450

fix tests

4146fb1

don't defer in a select

a6bfb05

add support for modular ticker, and ensure we use the same tomb so ac…

d4e088a

…quisition failure can bubble up

just a bit of logging

593de90

buixor approved these changes Oct 19, 2023

View reviewed changes

blotus and others added 3 commits November 22, 2023 10:12

Merge branch 'master' into loki

e85d98e

skip tests on windows

0dab9eb

fix test

695a104

buixor merged commit 92f923c into crowdsecurity:master Nov 22, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki integration #2 #2306

Loki integration #2 #2306

lperdereau commented Jun 24, 2023 •

edited

Loading

github-actions bot commented Jun 24, 2023

github-actions bot commented Jun 24, 2023

lperdereau commented Jun 24, 2023

codecov bot commented Jul 3, 2023 •

edited

Loading

mmoscher commented Aug 11, 2023

lperdereau commented Aug 11, 2023 •

edited

Loading

pschiffe commented Aug 28, 2023

lperdereau commented Sep 22, 2023

lperdereau commented Oct 2, 2023

buixor commented Oct 3, 2023

buixor left a comment

buixor Oct 3, 2023

lperdereau Oct 3, 2023

lperdereau Oct 12, 2023

lperdereau left a comment

lperdereau Oct 3, 2023

buixor commented Oct 16, 2023

buixor commented Oct 17, 2023 •

edited

Loading

buixor commented Oct 17, 2023

lperdereau commented Oct 18, 2023

buixor left a comment

Loki integration #2 #2306

Loki integration #2 #2306

Conversation

lperdereau commented Jun 24, 2023 • edited Loading

github-actions bot commented Jun 24, 2023

github-actions bot commented Jun 24, 2023

lperdereau commented Jun 24, 2023

codecov bot commented Jul 3, 2023 • edited Loading

Codecov Report

mmoscher commented Aug 11, 2023

lperdereau commented Aug 11, 2023 • edited Loading

pschiffe commented Aug 28, 2023

lperdereau commented Sep 22, 2023

lperdereau commented Oct 2, 2023

buixor commented Oct 3, 2023

buixor left a comment

Choose a reason for hiding this comment

buixor Oct 3, 2023

Choose a reason for hiding this comment

lperdereau Oct 3, 2023

Choose a reason for hiding this comment

lperdereau Oct 12, 2023

Choose a reason for hiding this comment

lperdereau left a comment

Choose a reason for hiding this comment

lperdereau Oct 3, 2023

Choose a reason for hiding this comment

buixor commented Oct 16, 2023

buixor commented Oct 17, 2023 • edited Loading

buixor commented Oct 17, 2023

lperdereau commented Oct 18, 2023

buixor left a comment

Choose a reason for hiding this comment

lperdereau commented Jun 24, 2023 •

edited

Loading

codecov bot commented Jul 3, 2023 •

edited

Loading

lperdereau commented Aug 11, 2023 •

edited

Loading

buixor commented Oct 17, 2023 •

edited

Loading