Promtail: Add new target for receiving GCP PubSub Push messages #6777

thepalbi · 2022-07-26T20:42:09Z

What this PR does / why we need it:
Promtail currently supports scraping GCP logs using a pull model, in which promtail subscribes to a GCP PubSub topic, to where logs are routed. There's another mechanism possible for receiving messages from PubSub topics (for example logs, but could potentially be other kind of messages) which is by using a push subscription.

This PR adds a new Promtail target which is able to receive messages from a GCP PubSub push subscription. It has been mainly thought for logs, but since it's generic it's able to support whatever message the GCP topic sends, since they all use a similar envelope. The target exposes and HTTP server (with just one POST /gcp/api/v1/push endpoint), which is then configured as target of this Push subscriptions.

Just to give an example, the envelope that GCP uses for this messages has the following shape:

{
	"message": {
		"attributes": {
			"logging.googleapis.com/timestamp": "2022-07-25T22:19:09.903683708Z"
		},
		"data": "ewogICJzZXZlcml0eSI6ICJERUJVRyIsCiAgInRleHRQYXlsb2FkIjogIkZ1bmN0aW9uIGV4ZWN1dGlvbiB0b29rIDExOTggbXMuIEZpbmlzaGVkIHdpdGggc3RhdHVzIGNvZGU6IDIwMCIsCiAgInRyYWNlIjogInByb2plY3RzL3dpcmVkLWhlaWdodC90cmFjZXMvNTRhYThhMjA4NzUyNTNjNmI0N2NhYThiZDI4YWQ2NTIiCn0=",
		"messageId": "5187581549398349",
		"message_id": "5187581549398349",
		"publishTime": "2022-07-25T22:19:15.56Z",
		"publish_time": "2022-07-25T22:19:15.56Z"
	},
	"subscription": "projects/wired-height-350515/subscriptions/test"
}

This message has been extracted from a push subscription, listening for logs from a cloud-function.

The target can be configured as follows:

  gcp_push:
    server:
      http_listen_address: 0.0.0.0
      http_listen_port: 8080
      log_level: debug
      register_instrumentation: true
    labels: 
      job: gcp_push_logs_test
    use_incoming_timestamp: true

This target exposes some of the received "label" as "internal labels" to be re-labeled by the user using the scrape config (much like the syslog target does), and dropped if not used. These are:

__gcp_message_id: message id
__gcp_attributes_*: All attributes read from .message.attributes are added as a temporal label, conveniently adapting the label name since it might contain non-loki-supported characters. For example for logging.googleapis.com/timestamp it's converted to __gcp_attributes_logging_googleapis_com_timestamp.

Some things to keep in mind:

GCP offers some nice delivery garantuees for not overloading the receiving sender like a sliding window algo with slow start.
It also offers acks and nacks, and retries, but because of promtail async processing, this are lost when the entry enters the underlying channel towards loki's client. The idea is once WAL-support is added, this will improve vastly.

Which issue(s) this PR fixes:
n/a

Special notes for your reviewer:
Documentation to be added.

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

grafanabot · 2022-07-26T20:57:01Z

./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%

clients/pkg/promtail/targets/serverutils/config.go

clients/pkg/promtail/targets/gcppush/target_test.go

clients/pkg/promtail/targets/gcppush/translation.go

grafanabot · 2022-07-26T21:08:56Z

./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 · 2022-07-27T07:57:43Z

@thepalbi thanks a ton for adding support for GCP pubsub push subscription type. Skimmed through the PR and overall looks great 👍

Just wanted to clarify one thing. Can we still keep single target type for GCP(gcplog) and just add extra config subscription_type to differentiate push and pull? (default being pull)? Rather than adding completely new target type.
I see we use more or less similar config for both except the server block for push subscription type.

We can keep everything more or less same, except if subscription_type: push we create newGCPpushTarget.

wdyt?

thepalbi · 2022-07-27T15:36:29Z

clients/pkg/promtail/scrapeconfig/scrapeconfig.go

@@ -37,7 +37,7 @@ type Config struct {
 	PipelineStages    stages.PipelineStages      `yaml:"pipeline_stages,omitempty"`
 	JournalConfig     *JournalTargetConfig       `yaml:"journal,omitempty"`
 	SyslogConfig      *SyslogTargetConfig        `yaml:"syslog,omitempty"`
-	GcplogConfig      *GcplogTargetConfig        `yaml:"gcplog,omitempty"`
+	GcplogConfig      *GCPLogTargetConfig        `yaml:"gcplog,omitempty"`


drive by: renamed the main target to use a better case

thepalbi · 2022-07-27T15:37:52Z

clients/pkg/promtail/targets/gcplog/pull_target.go

+}
+
+// nolint:revive
+func newGcplogTarget(


nit: maybe this one should also be renamed?

this is just returning the struct. why don't we make it as part of newPullTarget itself?. Any reason to have it as separate function?

thepalbi · 2022-07-27T15:39:42Z

clients/pkg/promtail/targets/gcplog/push_translation.go

+	// If the incoming request carries the tenant id, inject it as the reserved label, so it's used by the
+	// remote write client.
+	if xScopeOrgID != "" {
+		labels[lokiClient.ReservedLabelTenantID] = model.LabelValue(xScopeOrgID)
+	}


This functionality mimics what was done in d4cdc37. The idea is to make this target multitenant-supporting, so one can inject the tenantID from upstream using this reserved header (which is well-kown in loki world) and let the loki-client forward it when doing the remote write.

thepalbi · 2022-07-27T15:40:58Z

@thepalbi thanks a ton for adding support for GCP pubsub push subscription type. Skimmed through the PR and overall looks great 👍

Just wanted to clarify one thing. Can we still keep single target type for GCP(gcplog) and just add extra config subscription_type to differentiate push and pull? (default being pull)? Rather than adding completely new target type. I see we use more or less similar config for both except the server block for push subscription type.

We can keep everything more or less same, except if subscription_type: push we create newGCPpushTarget.

wdyt?

@kavirajk completely agree. Just merged both targets under the same package. Refactored (mainly renames) a little bit the Pull one so that both of them fit better. Let me know what you think 😀

grafanabot · 2022-07-27T19:08:02Z

./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

LGTM 🎉 thanks @thepalbi for addressing the comments.

Left few minor nits and suggestion. We are good to merge after that.

clients/pkg/promtail/targets/gcplog/push_translation.go

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

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

clients/pkg/promtail/targets/gcplog/pull_target.go

kavirajk · 2022-07-28T07:43:59Z

clients/pkg/promtail/targets/gcplog/pull_target.go

+}
+
+// nolint:revive
+func newGcplogTarget(


this is just returning the struct. why don't we make it as part of newPullTarget itself?. Any reason to have it as separate function?

kavirajk · 2022-07-28T11:54:36Z

@thepalbi also can you also please update the docs accordingly?
https://grafana.com/docs/loki/latest/clients/promtail/gcplog-cloud/

grafanabot · 2022-07-28T12:32:34Z

./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%

thepalbi · 2022-07-28T12:56:21Z

@thepalbi also can you also please update the docs accordingly? https://grafana.com/docs/loki/latest/clients/promtail/gcplog-cloud/

Yup, wanted to have a final 👍 on the implementation before doing this. Will add the docs so the PR is sounds!

grafanabot · 2022-07-28T13:07:38Z

./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%

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

thepalbi · 2022-07-28T17:54:55Z

@kavirajk added the docs on configuration and scraping sections as discussed. Also resolved all open comments. Can you take another pass + tag the correct docs person?

grafanabot · 2022-07-28T17:58:05Z

./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%

grafanabot · 2022-07-28T19:51:49Z

./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%

cstyan · 2022-07-29T00:03:37Z

Do we have any concerns about this target potentially allowing users to easily hit per stream rate limit issues? It looks to me like it would be very easy to configure many things to all write to one pubsub and have it all pushed to promtail as a single stream.

Should we consider enforcing some amount of the attributes being used as labels?

kavirajk · 2022-07-29T08:43:53Z

@cstyan that's the perfectly valid concern.

Even now user can take advantage of relabeling those gcp atrributes.
https://github.com/grafana/loki/blob/pablo%2Fgcp-push-target/clients/pkg/promtail/targets/gcplog/formatter.go#L60-L72

And you are right, user may not be aware about the impact before sending too much messages into single stream.

IMO, good place to add this note is in gcp-cloud setup doc https://grafana.com/docs/loki/latest/clients/promtail/gcplog-cloud/

I think @thepalbi decided to update the cloud setup doc in the follow up PR. Can you take care of that when creating follow up PR @thepalbi ?

Co-authored-by: Kaviraj Kanagaraj <[email protected]>

Co-authored-by: Karen Miller <[email protected]>

grafanabot · 2022-08-02T13:45:25Z

./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%

KMiller-Grafana

Thank you for revising the docs. The docs in this PR look good to me. I put in a suggestion for a typo fix, but I'm approving now so you won't need a re-review from me.

docs/sources/clients/promtail/configuration.md

Co-authored-by: Karen Miller <[email protected]>

grafanabot · 2022-08-02T16:28:55Z

./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%

jeschkies

Nice work!

jeschkies · 2022-08-02T16:28:16Z

clients/pkg/promtail/targets/gcplog/push_target.go

+}
+
+func (h *pushTarget) push(w http.ResponseWriter, r *http.Request) {
+	entries := h.handler.Chan()


I'm not sure the channel would change often. Why not h.entires?

You mean saving h.handler.Chan() as a struct field, and directly accessing it?

Sorry. I was out. Yes.

jeschkies · 2022-08-02T16:32:11Z

clients/pkg/promtail/targets/gcplog/push_target_test.go

+			serverConfig, port, err := getServerConfigWithAvailablePort()
+			require.NoError(t, err, "error generating server config or finding open port")
+			config := &scrapeconfig.GcplogTargetConfig{
+				Server:               serverConfig,
+				Labels:               tc.args.Labels,
+				UseIncomingTimestamp: false,
+				SubscriptionType:     "push",
+			}


Running the server is quite a handful. What do you think about testing the handler function push with a ResponseRecorder instead?

clients/pkg/promtail/targets/gcplog/push_translation.go

thepalbi · 2022-08-02T19:04:01Z

Thanks for the look @jeschkies , I'll address the comments in a follow up 😀

**What this PR does / why we need it**: This PR includes some pending improvements to newish promtail GCP push target: - Pending low hanging optimization for replacing a regex that gets evaluated on every incoming request from GCP, for a simpler solution. [Related comments](#6777 (comment)) - Some small refactors. See: [this](#6777 (review)) - Add some validations over the payload the target receives. Payload details are [here](https://cloud.google.com/pubsub/docs/push). This PR adds just validation over the log line, the incoming GCP message ID to add some traceability if needed, and the subscription name because it can be used as label. - Add `subscription` GCP payload field as label, which includes the PubSub subscription identifier. - Add `cause` label to error metrics. Since there's multiple failure point this might help debugging / notice strange behaviours. Also, by running the push target in some dev workloads, we noticed it has some labels different to the ones added in the pull target. This PR also refactors the logic to re-use the same GCP log entry parsing logic in both targets. That way the labels exposed to the user in the push target are $Labels_{pull} \cup Labels_{push}$. **Which issue(s) this PR fixes**: Related to: grafana/cloud-onboarding#2067 **Special notes for your reviewer**: This PR is a follow up [this other one](#6777). It works on some comments made after merge, and adds some changes we saw necessary upon running this target in real environments.  **Checklist** - [x] Documentation added - [x] 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` Co-authored-by: Kaviraj Kanagaraj <[email protected]>

…ana#6777)

**What this PR does / why we need it**: This PR includes some pending improvements to newish promtail GCP push target: - Pending low hanging optimization for replacing a regex that gets evaluated on every incoming request from GCP, for a simpler solution. [Related comments](grafana#6777 (comment)) - Some small refactors. See: [this](grafana#6777 (review)) - Add some validations over the payload the target receives. Payload details are [here](https://cloud.google.com/pubsub/docs/push). This PR adds just validation over the log line, the incoming GCP message ID to add some traceability if needed, and the subscription name because it can be used as label. - Add `subscription` GCP payload field as label, which includes the PubSub subscription identifier. - Add `cause` label to error metrics. Since there's multiple failure point this might help debugging / notice strange behaviours. Also, by running the push target in some dev workloads, we noticed it has some labels different to the ones added in the pull target. This PR also refactors the logic to re-use the same GCP log entry parsing logic in both targets. That way the labels exposed to the user in the push target are $Labels_{pull} \cup Labels_{push}$. **Which issue(s) this PR fixes**: Related to: https://github.com/grafana/cloud-onboarding/issues/2067 **Special notes for your reviewer**: This PR is a follow up [this other one](grafana#6777). It works on some comments made after merge, and adds some changes we saw necessary upon running this target in real environments.  **Checklist** - [x] Documentation added - [x] 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` Co-authored-by: Kaviraj Kanagaraj <[email protected]>

pull-request-size bot added the size/XL label Jul 26, 2022

thepalbi commented Jul 26, 2022

View reviewed changes

thepalbi force-pushed the pablo/gcp-push-target branch from 8c65211 to 95b9b60 Compare July 26, 2022 21:03

thepalbi marked this pull request as ready for review July 26, 2022 21:19

thepalbi requested a review from a team as a code owner July 26, 2022 21:19

pull-request-size bot added size/XXL and removed size/XL labels Jul 27, 2022

thepalbi commented Jul 27, 2022

View reviewed changes

thepalbi force-pushed the pablo/gcp-push-target branch from ff3faf7 to 9f481cd Compare July 27, 2022 19:03

kavirajk reviewed Jul 28, 2022

View reviewed changes

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

thepalbi requested a review from KMiller-Grafana as a code owner July 28, 2022 17:53

github-actions bot added the area/docs label Jul 28, 2022

thepalbi force-pushed the pablo/gcp-push-target branch 2 times, most recently from 35446db to d461e11 Compare July 28, 2022 19:47

thepalbi and others added 16 commits August 2, 2022 10:41

Address github comments

6f9548d

make actual target impls package private

ef23e23

fix lint

031844b

Update clients/pkg/promtail/targets/gcplog/target.go

b79387d

Co-authored-by: Kaviraj Kanagaraj <[email protected]>

fix test

258a3a8

added config section

5b4b5a8

added scraping docs

69a2bec

Make everything as backwards compatible as possible

4938dd8

Update docs/sources/clients/promtail/configuration.md

1020b26

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/configuration.md

8232428

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/configuration.md

91dc153

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/configuration.md

6052172

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/configuration.md

d97ec67

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/configuration.md

2803829

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/scraping.md

eb77eb2

Co-authored-by: Karen Miller <[email protected]>

Update docs/sources/clients/promtail/scraping.md

1dfa0dd

Co-authored-by: Karen Miller <[email protected]>

thepalbi force-pushed the pablo/gcp-push-target branch from bf843ef to 1dfa0dd Compare August 2, 2022 13:41

KMiller-Grafana approved these changes Aug 2, 2022

View reviewed changes

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

Update docs/sources/clients/promtail/configuration.md

979cdea

Co-authored-by: Karen Miller <[email protected]>

kavirajk merged commit c9ef062 into main Aug 2, 2022

kavirajk deleted the pablo/gcp-push-target branch August 2, 2022 16:35

jeschkies requested changes Aug 2, 2022

View reviewed changes

thepalbi mentioned this pull request Sep 5, 2022

Improvements to Promtail GCP push target #7057

Merged

4 tasks

osg-grafana removed the area/docs label Oct 19, 2022

lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022

Promtail: Add new target for receiving GCP PubSub Push messages (graf…

d4c2664

…ana#6777)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promtail: Add new target for receiving GCP PubSub Push messages #6777

Promtail: Add new target for receiving GCP PubSub Push messages #6777

thepalbi commented Jul 26, 2022 •

edited

Loading

grafanabot commented Jul 26, 2022

grafanabot commented Jul 26, 2022

kavirajk commented Jul 27, 2022

thepalbi Jul 27, 2022

thepalbi Jul 27, 2022

kavirajk Jul 28, 2022

thepalbi Jul 27, 2022

thepalbi commented Jul 27, 2022

grafanabot commented Jul 27, 2022

kavirajk left a comment

kavirajk Jul 28, 2022

kavirajk commented Jul 28, 2022

grafanabot commented Jul 28, 2022

thepalbi commented Jul 28, 2022

grafanabot commented Jul 28, 2022

thepalbi commented Jul 28, 2022

grafanabot commented Jul 28, 2022

grafanabot commented Jul 28, 2022

cstyan commented Jul 29, 2022

kavirajk commented Jul 29, 2022

grafanabot commented Aug 2, 2022

KMiller-Grafana left a comment

grafanabot commented Aug 2, 2022

jeschkies left a comment

jeschkies Aug 2, 2022

thepalbi Aug 3, 2022

jeschkies Aug 9, 2022

jeschkies Aug 2, 2022

thepalbi commented Aug 2, 2022

Promtail: Add new target for receiving GCP PubSub Push messages #6777

Promtail: Add new target for receiving GCP PubSub Push messages #6777

Conversation

thepalbi commented Jul 26, 2022 • edited Loading

grafanabot commented Jul 26, 2022

grafanabot commented Jul 26, 2022

kavirajk commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thepalbi commented Jul 27, 2022

grafanabot commented Jul 27, 2022

kavirajk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavirajk commented Jul 28, 2022

grafanabot commented Jul 28, 2022

thepalbi commented Jul 28, 2022

grafanabot commented Jul 28, 2022

thepalbi commented Jul 28, 2022

grafanabot commented Jul 28, 2022

grafanabot commented Jul 28, 2022

cstyan commented Jul 29, 2022

kavirajk commented Jul 29, 2022

grafanabot commented Aug 2, 2022

KMiller-Grafana left a comment

Choose a reason for hiding this comment

grafanabot commented Aug 2, 2022

jeschkies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thepalbi commented Aug 2, 2022

thepalbi commented Jul 26, 2022 •

edited

Loading