Skip to content
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

feat(loki-canary): Add support to push logs directly to Loki. #7063

Merged
merged 16 commits into from
Sep 9, 2022

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented Sep 6, 2022

What this PR does / why we need it:
Add additional push mode to loki-canary, which pushes the logs directly to given Loki URL as it generates logs.

The real function of Loki Canary is to act like a tenant and help us know the whether Loki installation is working as perceived by a real tenant.

Main rationale for this additional push mode is to make canary more standalone without needing for promtail (or grafana-agent) to scrape it's logs and send to loki, with this change, Loki canary happily tests Loki behavior without needing any other dependencies.

NOTE:
If you run Loki behind any proxy that has different authorization policies to READ or WRITE to Loki, then important change that canary operator need to be aware of it, now the user credentials they pass via -user and -pass to access loki endpoints need to have both read' and write` permissions (previously canary just query the logs where as promtail is the one pushes the logs, so just READ permissions was sufficient).

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
There will be follow up PR(s) to cleanup, particularly reader and comparitor component in terms of logging with proper logger. Rationale is to keep the changes small per PR to make it easy to review.

This changes were tested it in one of the internal Loki dev cell.

This PR is a no-op if this new push flag is disabled (it's disabled by default)

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

Basic working version

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk kavirajk force-pushed the kavirajk/support-push-mode-canary branch from 997660d to 3ef4eb9 Compare September 6, 2022 07:05
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Sep 7, 2022
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk kavirajk force-pushed the kavirajk/support-push-mode-canary branch from caed569 to 4f840e9 Compare September 7, 2022 15:04
@kavirajk kavirajk marked this pull request as ready for review September 7, 2022 15:05
@kavirajk kavirajk requested a review from a team as a code owner September 7, 2022 15:05
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised a couple points here, let's address before we merge because I see you already have one approval.

cmd/loki-canary/main.go Show resolved Hide resolved
cmd/loki-canary/main.go Outdated Show resolved Hide resolved
pkg/canary/writer/push.go Outdated Show resolved Hide resolved
pkg/canary/writer/push.go Show resolved Hide resolved
pkg/canary/writer/push.go Outdated Show resolved Hide resolved
pkg/canary/writer/push_test.go Show resolved Hide resolved
kavirajk and others added 2 commits September 8, 2022 10:19
Co-authored-by: Danny Kopping <danny.kopping@grafana.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0.4%
+               loki	0%

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kavirajk! One comment about logging, then I think we're good to go

pkg/canary/writer/push.go Show resolved Hide resolved
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Sep 8, 2022
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

pkg/canary/writer/push.go Show resolved Hide resolved
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

1 similar comment
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@kavirajk kavirajk merged commit 2c9fa05 into main Sep 9, 2022
@kavirajk kavirajk deleted the kavirajk/support-push-mode-canary branch September 9, 2022 08:33
lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022
…a#7063)


Add additional `push` mode to loki-canary, which pushes the logs
directly to given Loki URL as it generates logs.

The real function of Loki Canary is to act like a tenant and help us
know the whether Loki installation is working as perceived by a real
tenant.

Main rationale for this additional push mode is to make canary more
standalone without needing for `promtail` (or `grafana-agent`) to scrape
it's logs and send to loki, with this change, Loki canary happily tests
Loki behavior without needing any other dependencies.

**NOTES**:
1. If you run Loki behind any proxy that has different authorization
policies to READ or WRITE to Loki, then important change that canary
operator need to be aware of it, now the user credentials they pass via
`-user` and `-pass` to access loki endpoints need to have both `read'
and `write` permissions (previously canary just query the logs where as
promtail is the one pushes the logs, so just READ permissions was
sufficient).

2. There will be follow up PR(s) to cleanup, particularly `reader` and
`comparitor` component in terms of logging with proper logger. Rationale
is to keep the changes small per PR to make it easy to review.

3. This changes were tested it in one of the internal Loki dev cell.

4. **This PR is a no-op if this new `push` flag is disabled (it's disabled
by default)**

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Co-authored-by: Danny Kopping <danny.kopping@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants