-
Notifications
You must be signed in to change notification settings - Fork 90
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.
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
app/log: add loki client #1337
app/log: add loki client #1337
Conversation
Codecov ReportBase: 53.37% // Head: 53.44% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1337 +/- ##
==========================================
+ Coverage 53.37% 53.44% +0.07%
==========================================
Files 145 147 +2
Lines 18193 18316 +123
==========================================
+ Hits 9710 9789 +79
- Misses 7145 7178 +33
- Partials 1338 1349 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
568a0a1
to
3e58dc6
Compare
Line: line, | ||
}) | ||
if batch.Size() > c.batchMax { | ||
batch = newBatch(c.service) // Just silently drop, there should have been multiple error logs below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dropping complete batch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for simplicity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per package godoc:
// It is best-effort, meaning it doesn't provide delivery guarantees, it will drop logs if
// loki isn't accessible. It is meant to be used in local dev environments or where log
// delivery isn't critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need guarantees, then use proper 12factor app patterns and official tools.
app/log/loki/client.go
Outdated
case <-ticker.C: | ||
// Do not send if the batch is too young | ||
if batch.Age() < c.batchWait { | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be continue rather than break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Add simple loki client to directly sending logs to loki instead of jumping through docker-compose hoops. Still needs to be integrated.
category: feature
ticket: #1406