Loki: Modified heroku drain target to make any url query parameters available as labels #7619

cadrake · 2022-11-07T19:57:17Z

What this PR does / why we need it:

This PR reads the url query parameters included in Heroku drain POST request and creates __param_<name> labels similar to how the parameters work with prometheus metrics and metric endpoints as a way to allow setting the true application name when ingesting logs from a Heroku drain.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

The existing app field coming from Heroku only specifies application source (either the application itself or Heroku) and thus is insufficient for uniquely identifying a log lines source.

Checklist

Reviewed the CONTRIBUTING.md guide
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

…le as labels

CLAassistant · 2022-11-07T19:57:29Z

All committers have signed the CLA.

grafanabot · 2022-11-07T20:01:54Z

./tools/diff_coverage.sh ../loki-target-branch/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.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0.4%
+               loki	0%

cadrake · 2022-11-07T20:12:12Z

Note that I didnt update documentation because the section on Heroku drains already mentions url parameters being available as __param_<name> labels; however, that looks to have possibly been a typo or copied from another section and not updated.

thepalbi · 2022-11-09T21:25:07Z

Note that I didnt update documentation because the section on Heroku drains already mentions url parameters being available as __param_<name> labels; however, that looks to have possibly been a typo or copied from another section and not updated.

Hi @cadrake , thanks for bringing this up. As for what you mention in the docs, I think you refer to this:

The _param label is set to the value of the first passed URL parameter called .

That quoted sentence comes from the relabel_configs section, and it refers that if you use a relabel_config with __param_<name> it will affect how the URL takes shape in the scraped targets. Almost sure this doesn't apply in this case, but I agree the documentation is kid of misleading, will take this back and correct it.

However, I think what you brought up in this PR is a nice addition to the Heroku target. Commenting on the PR.

thepalbi

In order for this PR to be sound, we need some other changes:

Update docs here and here adding the new available labels
Add a test case covering query params being extracted AND dropped
Add entry to changelog here

clients/pkg/promtail/targets/heroku/target.go

grafanabot · 2022-11-09T21:38:46Z

./tools/diff_coverage.sh ../loki-target-branch/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%

grafanabot · 2022-11-10T16:51:03Z

./tools/diff_coverage.sh ../loki-target-branch/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%

cadrake · 2022-11-10T17:07:13Z

For the unit tests, can I piggy-back off of the existing tests or would you like me to create new ones?

thepalbi · 2022-11-10T17:51:06Z

For the unit tests, can I piggy-back off of the existing tests or would you like me to create new ones?

I think copying one of these as a new test exercising the URL with query params will do

cadrake · 2022-11-10T18:27:23Z

I ended up expanding TestHerokuDrainTarget to run additional tests with and without query parameters, also updated all the documentation and changelog

grafanabot · 2022-11-10T18:33:37Z

./tools/diff_coverage.sh ../loki-target-branch/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%

thepalbi

LGTM. Left some nits regarding extra-spaces. Will bump this through Loki to get a review from them, since it's required.

docs/sources/clients/promtail/scraping.md

docs/sources/clients/promtail/configuration.md

grafanabot · 2022-11-10T22:37:42Z

./tools/diff_coverage.sh ../loki-target-branch/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.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-11-27T15:45:06Z

./tools/diff_coverage.sh ../loki-target-branch/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%

grafanabot · 2022-11-30T18:49:11Z

./tools/diff_coverage.sh ../loki-target-branch/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%

slim-bean · 2022-12-01T20:58:13Z

clients/pkg/promtail/targets/heroku/target.go

+		// Create __heroku_drain_param_<name> labels from query parameters
+		params := r.URL.Query()
+		for k, v := range params {
+			lb.Set(fmt.Sprintf("__heroku_drain_param_%s", k), strings.Join(v, ","))


Is there a reason to prefix the keys? would it be better for users to just use the url param key as the label name?

oh I see, I think the intent is to use a relabel rule to turn these into actual labels sent to Loki?

That seems reasonable to me 👍

slim-bean

LGTM, thank you!!

Modified heroku drain target to make any url query parameters availab…

b248cb0

…le as labels

cadrake requested a review from a team as a code owner November 7, 2022 19:57

pull-request-size bot added the size/XS label Nov 7, 2022

cadrake changed the title ~~Modified heroku drain target to make any url query parameters available as labels~~ Loki: Modified heroku drain target to make any url query parameters available as labels Nov 7, 2022

thepalbi requested changes Nov 9, 2022

View reviewed changes

clients/pkg/promtail/targets/heroku/target.go Outdated Show resolved Hide resolved

Renaming query parameter labels to match existing heroku drain labels

71e2e17

Updated changelog, documentation and unit tests

7d0dba6

pull-request-size bot added size/L and removed size/XS labels Nov 10, 2022

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Nov 10, 2022

thepalbi approved these changes Nov 10, 2022

View reviewed changes

docs/sources/clients/promtail/scraping.md Show resolved Hide resolved

docs/sources/clients/promtail/scraping.md Outdated Show resolved Hide resolved

docs/sources/clients/promtail/configuration.md Outdated Show resolved Hide resolved

Restored whitespace to clean up git diff

2bcd70f

Updated comment to match code

0019fc1

Merge branch 'main' into adrake/heroku-labels-from-http-request

d2703a8

espendk mentioned this pull request Nov 30, 2022

Add Logplex-Drain-Token to labels in heroku_drain #7818

Open

slim-bean reviewed Dec 1, 2022

View reviewed changes

slim-bean approved these changes Dec 1, 2022

View reviewed changes

slim-bean merged commit ea9ad33 into grafana:main Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Modified heroku drain target to make any url query parameters available as labels #7619

Loki: Modified heroku drain target to make any url query parameters available as labels #7619

cadrake commented Nov 7, 2022 •

edited

Loading

CLAassistant commented Nov 7, 2022 •

edited

Loading

grafanabot commented Nov 7, 2022

cadrake commented Nov 7, 2022

thepalbi commented Nov 9, 2022 •

edited

Loading

thepalbi left a comment

grafanabot commented Nov 9, 2022

grafanabot commented Nov 10, 2022

cadrake commented Nov 10, 2022

thepalbi commented Nov 10, 2022

cadrake commented Nov 10, 2022

grafanabot commented Nov 10, 2022

thepalbi left a comment

grafanabot commented Nov 10, 2022

grafanabot commented Nov 27, 2022

grafanabot commented Nov 30, 2022

slim-bean Dec 1, 2022

slim-bean Dec 1, 2022

slim-bean left a comment

Loki: Modified heroku drain target to make any url query parameters available as labels #7619

Loki: Modified heroku drain target to make any url query parameters available as labels #7619

Conversation

cadrake commented Nov 7, 2022 • edited Loading

CLAassistant commented Nov 7, 2022 • edited Loading

grafanabot commented Nov 7, 2022

cadrake commented Nov 7, 2022

thepalbi commented Nov 9, 2022 • edited Loading

thepalbi left a comment

Choose a reason for hiding this comment

grafanabot commented Nov 9, 2022

grafanabot commented Nov 10, 2022

cadrake commented Nov 10, 2022

thepalbi commented Nov 10, 2022

cadrake commented Nov 10, 2022

grafanabot commented Nov 10, 2022

thepalbi left a comment

Choose a reason for hiding this comment

grafanabot commented Nov 10, 2022

grafanabot commented Nov 27, 2022

grafanabot commented Nov 30, 2022

slim-bean Dec 1, 2022

Choose a reason for hiding this comment

slim-bean Dec 1, 2022

Choose a reason for hiding this comment

slim-bean left a comment

Choose a reason for hiding this comment

cadrake commented Nov 7, 2022 •

edited

Loading

CLAassistant commented Nov 7, 2022 •

edited

Loading

thepalbi commented Nov 9, 2022 •

edited

Loading