Logproto: Extract push.proto from logproto package to the separate module #8259

mar4uk · 2023-01-24T15:47:17Z

What this PR does / why we need it:
This PR is aimed to extract push proto to a separate module. It will allow using push proto in other repositories without bringing the whole Loki as a dependency.

Special notes for your reviewer:

Checklist

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

grafanabot · 2023-01-25T16:00:45Z

./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 · 2023-01-25T17:05:17Z

./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 · 2023-01-25T17:20:35Z

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

owen-d

I like this PR, but I think we'll have to address the proto package name changing and breaking method compatibility. See #6852 for another example of this in the past.

Hopefully we can have separate golang pkgs (the new push pkg and the pre-existing logproto) both register the same grpc package namespace (logproto). If that doesn't work, we could look at code-generating a compatible grpc package from the same definition subset.

owen-d · 2023-01-25T21:23:17Z

pkg/push/push.pb.go

+
+func (c *pusherClient) Push(ctx context.Context, in *PushRequest, opts ...grpc.CallOption) (*PushResponse, error) {
+	out := new(PushResponse)
+	err := c.cc.Invoke(ctx, "/push.Pusher/Push", in, out, opts...)


I'm worried this may be a breaking change since the path is used in the method name here.

owen-d · 2023-01-25T21:25:42Z

pkg/push/push.proto

@@ -0,0 +1,40 @@
+syntax = "proto3";
+
+package push;


To address breaking concerns, I wonder if we can name this with the same package as before: package logproto;

it seems working! renamed package from push to logproto

grafanabot · 2023-01-26T16:18:17Z

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

owen-d

Nice work. LGTM!

grafanabot · 2023-01-26T21:59:25Z

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

**What this PR does / why we need it**: In #9694, we modified the `push` pkg to import Prometheus as a dependency to use the `labels.Labels` type for the entries' non-indexed labels. Having Prometheus as a dependency is problematic since any project importing the `push` pkg will need to import Prometheus as a result. In fact, this is one of the reasons why the `push` pkg was extracted from Loki in the first place (#8259). This PR removes the dependency of Prometheus from the `push` pkg by copying some bits of the implementation for Prometheus' `labels.Labels`. We copy: - The Labels struct definition - The JSON Marshaling and Unmarshaling methods for the labels. We need this so labels are encoded as maps instead of an array of objects. --- **Notes for reviewers:** - To implement the JSON Marshaling and Unmarshaling methods the `push` pkg now depends on `golang.org/x/exp`. I think it should be fine for projects importing the `push` pkg to also depend on `golang.org/x/exp`, right?

**What this PR does / why we need it**: In #9694, we modified the `push` pkg to import Prometheus as a dependency to use the `labels.Labels` type for the entries' non-indexed labels. Having Prometheus as a dependency is problematic since any project importing the `push` pkg will need to import Prometheus as a result. In fact, this is one of the reasons why the `push` pkg was extracted from Loki in the first place (#8259). This PR removes the dependency of Prometheus from the `push` pkg by copying some bits of the implementation for Prometheus' `labels.Labels`. We copy: - The Labels struct definition - The JSON Marshaling and Unmarshaling methods for the labels. We need this so labels are encoded as maps instead of an array of objects. --- **Notes for reviewers:** - To implement the JSON Marshaling and Unmarshaling methods the `push` pkg now depends on `golang.org/x/exp`. I think it should be fine for projects importing the `push` pkg to also depend on `golang.org/x/exp`, right? (cherry picked from commit 15af77b)

Backport 15af77b from #9937 --- **What this PR does / why we need it**: In #9694, we modified the `push` pkg to import Prometheus as a dependency to use the `labels.Labels` type for the entries' non-indexed labels. Having Prometheus as a dependency is problematic since any project importing the `push` pkg will need to import Prometheus as a result. In fact, this is one of the reasons why the `push` pkg was extracted from Loki in the first place (#8259). This PR removes the dependency of Prometheus from the `push` pkg by copying some bits of the implementation for Prometheus' `labels.Labels`. We copy: - The Labels struct definition - The JSON Marshaling and Unmarshaling methods for the labels. We need this so labels are encoded as maps instead of an array of objects. --- **Notes for reviewers:** - To implement the JSON Marshaling and Unmarshaling methods the `push` pkg now depends on `golang.org/x/exp`. I think it should be fine for projects importing the `push` pkg to also depend on `golang.org/x/exp`, right? Co-authored-by: Salva Corts <[email protected]>

pull-request-size bot added the size/XXL label Jan 24, 2023

mar4uk changed the title ~~Logproto: Extract push.proto from logproto.proto~~ Logproto: Extract push.proto from logproto package to the separate module Jan 24, 2023

mar4uk force-pushed the extract-push-proto-from-logproto branch 11 times, most recently from 29c50af to f0a28f9 Compare January 25, 2023 15:54

mar4uk force-pushed the extract-push-proto-from-logproto branch from f0a28f9 to 6c41f23 Compare January 25, 2023 17:00

mar4uk force-pushed the extract-push-proto-from-logproto branch from 6c41f23 to efafb94 Compare January 25, 2023 17:15

mar4uk marked this pull request as ready for review January 25, 2023 20:56

mar4uk requested a review from a team as a code owner January 25, 2023 20:56

owen-d reviewed Jan 25, 2023

View reviewed changes

Extract push.proto from logproto.proto

2f958b1

mar4uk force-pushed the extract-push-proto-from-logproto branch from efafb94 to b012f96 Compare January 26, 2023 16:13

rename push.proto package from push to logproto

b012f96

mar4uk requested a review from owen-d January 26, 2023 16:27

owen-d approved these changes Jan 26, 2023

View reviewed changes

Merge branch 'main' into extract-push-proto-from-logproto

616a813

owen-d merged commit 4cd1246 into grafana:main Jan 26, 2023

mar4uk mentioned this pull request Jan 27, 2023

[chore] update pkg/push version #8312

Merged

5 tasks

salvacorts mentioned this pull request Jul 14, 2023

Remove Prometheus dependency for push pkg #9937

Merged

grafanabot mentioned this pull request Jul 17, 2023

[k159] Remove Prometheus dependency for push pkg #9956

Merged

kavirajk mentioned this pull request Jul 24, 2023

chore(loki): Have fewer go.mod and go.sum #10023

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logproto: Extract push.proto from logproto package to the separate module #8259

Logproto: Extract push.proto from logproto package to the separate module #8259

mar4uk commented Jan 24, 2023 •

edited

Loading

grafanabot commented Jan 25, 2023

grafanabot commented Jan 25, 2023

grafanabot commented Jan 25, 2023

owen-d left a comment

owen-d Jan 25, 2023

owen-d Jan 25, 2023

mar4uk Jan 26, 2023

grafanabot commented Jan 26, 2023

owen-d left a comment

grafanabot commented Jan 26, 2023

Logproto: Extract push.proto from logproto package to the separate module #8259

Logproto: Extract push.proto from logproto package to the separate module #8259

Conversation

mar4uk commented Jan 24, 2023 • edited Loading

grafanabot commented Jan 25, 2023

grafanabot commented Jan 25, 2023

grafanabot commented Jan 25, 2023

owen-d left a comment

Choose a reason for hiding this comment

owen-d Jan 25, 2023

Choose a reason for hiding this comment

owen-d Jan 25, 2023

Choose a reason for hiding this comment

mar4uk Jan 26, 2023

Choose a reason for hiding this comment

grafanabot commented Jan 26, 2023

owen-d left a comment

Choose a reason for hiding this comment

grafanabot commented Jan 26, 2023

mar4uk commented Jan 24, 2023 •

edited

Loading