New logs service with GetLogs API stub #9738

zmajeed · 2024-02-14T20:14:50Z

Minimal changes for new logs service with single GetLogs API that has all the messages and types defined in logs.proto but only returns a dummy response for simple testing

jrockway

Looks good.

bazel run //:gazelle is necessary. After that, expose logs.proto to the proto generator by adding a rule in src/logs/BUILD.bazel like:

filegroup(
    name = "protos",
    srcs = glob(["*.proto"]),
    visibility = ["//src:__pkg__"],
)

Then in src/BUILD.bazel, add //src/logs:protos to the srcs of all_protos. Run bazel run //:buildifier to make sure the srcs list is sorted (otherwise //:buildifier_test will fail).

robert-uhl · 2024-02-15T13:57:12Z

src/client/client.go

@@ -85,6 +86,9 @@ type TransactionAPIClient transaction.APIClient
 // DebugClient is an alias of debug.DebugClient
 type DebugClient debug.DebugClient

+// LogsClient is an alias of logs.APIClient


I would not add anything to the external client library. Users should just access our API via the Go gRPC bindings.

robert-uhl · 2024-02-15T13:59:05Z

src/internal/client/client.go

@@ -98,6 +102,7 @@ type APIClient struct {
 	AdminAPIClient
 	TransactionAPIClient
 	DebugClient
+	LogsClient


I know that you’re just copying the existing pattern here, which is fine, but I think that you should consider just adding a member rather than embedding here. I don’t think that there’s a need for embedding in this case — it just means being able to call logs.APIClient methods on a Client instance, when they don’t overlap with some other embedded type’s methods.

no client changes now

robert-uhl · 2024-02-15T14:01:26Z

src/logs/logs.proto

+message ParsedJSONLogMessage {
+    VerbatimLogMessage verbatim = 1; // The verbatim line from Loki.
+    //map<string, google.protobuf.Any> fields = 2; // A raw JSON parse of the entire line.
+    map<string, string> fields = 2; // A raw JSON parse of the entire line.


The assumption here is that the JSON property values will always be strings. Is that guaranteed?

changed to protobuf Struct

codecov · 2024-02-15T14:20:50Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (faf04ed) 59.13% compared to head (8ce081c) 59.01%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/client/transaction.gen.go	0.00%	2 Missing ⚠️
src/internal/client/transaction.gen.go	0.00%	2 Missing ⚠️
src/internal/pachd/builder.go	66.66%	1 Missing and 1 partial ⚠️
src/internal/restgateway/restgateway.go	50.00%	1 Missing ⚠️
src/internal/testpachd/mock_pachd.go	85.71%	1 Missing ⚠️
src/server/logs/server/api_server.go	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9738      +/-   ##
==========================================
- Coverage   59.13%   59.01%   -0.12%     
==========================================
  Files         582      583       +1     
  Lines       69908    69941      +33     
==========================================
- Hits        41338    41278      -60     
- Misses      27987    28089     +102     
+ Partials      583      574       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bbonenfant

No issues with this, but a couple of requests/questions:

Is this intended to be customer facing? Should the client expose this? Any new APIs (proto files) need to be manually exposed within the python client.

if Yes: then make a ticket with the INT team so that we can get this exposed and maybe tested,
If No: then I can show you where to disable the python generation for this file.

Please move the comments/documentation to be above the fields. That's where the parser expects the documentation to be. Being inline now, none of this important information is making it into the generated python code.

bbonenfant · 2024-02-15T19:09:22Z

src/logs/logs.proto

+   LOG_FORMAT_PPS_LOGMESSAGE = 3;
+}
+
+message GetLogsRequest {


As this is the "entrance point" to the new RPC, could each of these fields get a little documentation? Additionally, could you move the comment above the field instead of inline? It makes the file longer, but the parser expects documentation for fields to be the line(s) immediately above the field definition.

bbonenfant · 2024-02-15T19:17:58Z

src/logs/logs.proto

+    string job = 2;
+}
+
+message PipelineDatumLogQuery {


Is this actually used anywhere?

zmajeed · 2024-02-22T16:09:56Z

will add more documentation in following PRs

FahadBSyed · 2024-02-22T16:32:33Z

Could you provide some context on the reasoning or motivation behind this change? Other than that, the code itself looks good.

zmajeed · 2024-02-22T16:35:15Z

It's a GetLogs API improvement

zmajeed requested review from a team as code owners February 14, 2024 20:14

zmajeed requested review from avigil and bbonenfant February 14, 2024 20:14

zmajeed assigned jrockway, robert-uhl and acohen4 Feb 14, 2024

zmajeed marked this pull request as draft February 14, 2024 20:22

jrockway requested review from jrockway and removed request for avigil February 14, 2024 20:23

jrockway approved these changes Feb 14, 2024

View reviewed changes

zmajeed marked this pull request as ready for review February 14, 2024 22:35

robert-uhl reviewed Feb 15, 2024

View reviewed changes

bbonenfant approved these changes Feb 15, 2024

View reviewed changes

zmajeed force-pushed the zmajeed_logs_service_api_1 branch from 1109060 to cc75d03 Compare February 21, 2024 15:34

zmajeed requested a review from a team as a code owner February 21, 2024 15:34

zmajeed requested a review from FahadBSyed February 21, 2024 15:34

zmajeed force-pushed the zmajeed_logs_service_api_1 branch 2 times, most recently from 2fe663f to 5db2a45 Compare February 21, 2024 19:24

zmajeed added 7 commits February 22, 2024 07:35

New logs service with GetLogs API stub

4997d1b

fix mock logs server for test

155d270

bazel build updates

9968739

add proto generated files for logs service

83ae8ef

don't add logs client to pach client

0d540da

change inline comments to single-lines

59e738f

remove temp file

8866685

zmajeed force-pushed the zmajeed_logs_service_api_1 branch from e8dc9c1 to 8866685 Compare February 22, 2024 14:11

zmajeed added 3 commits February 22, 2024 10:00

bazel

73beaf7

remove unused proto type

30de747

bazel

8ce081c

FahadBSyed approved these changes Feb 22, 2024

View reviewed changes

zmajeed merged commit 1fb5812 into master Feb 22, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New logs service with GetLogs API stub #9738

New logs service with GetLogs API stub #9738

zmajeed commented Feb 14, 2024 •

edited

jrockway left a comment

robert-uhl Feb 15, 2024

robert-uhl Feb 15, 2024

zmajeed Feb 22, 2024

robert-uhl Feb 15, 2024

zmajeed Feb 22, 2024

codecov bot commented Feb 15, 2024 •

edited

bbonenfant left a comment

bbonenfant Feb 15, 2024

bbonenfant Feb 15, 2024

zmajeed Feb 22, 2024

zmajeed commented Feb 22, 2024

FahadBSyed commented Feb 22, 2024

zmajeed commented Feb 22, 2024

New logs service with GetLogs API stub #9738

New logs service with GetLogs API stub #9738

Conversation

zmajeed commented Feb 14, 2024 • edited

jrockway 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

Choose a reason for hiding this comment

codecov bot commented Feb 15, 2024 • edited

Codecov Report

bbonenfant 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

zmajeed commented Feb 22, 2024

FahadBSyed commented Feb 22, 2024

zmajeed commented Feb 22, 2024

zmajeed commented Feb 14, 2024 •

edited

codecov bot commented Feb 15, 2024 •

edited