[ENH] Push logs with grpc error handling #1749

weiligu · 2024-02-21T20:58:53Z

Description of changes

https://linear.app/trychroma/issue/CHR-295/push-log-api

PushLogs implementation in DAO
PushLogs API for gRPC
DB error handling and retry is not included

Test plan

How are these changes tested?

record_log_service test
record_log test

weiligu · 2024-02-21T20:59:04Z

Current dependencies on/for this PR:

[ENH] Server side pull logs #1762
[ENH] Push logs with grpc error handling #1749 👈
main

This stack of pull requests is managed by Graphite.

github-actions · 2024-02-21T20:59:31Z

HammadB · 2024-02-21T21:17:03Z

idl/chromadb/proto/logservice.proto


+message PushLogsResponse {


This should have error types propagated in the response.

does grpc messages not have error types?

I can try to write a test to verify

It does, but we should expand, make sure calling code propagates and add message/status etc

https://cloud.google.com/apis/design/errors#error_model

chroma/idl/chromadb/proto/chroma.proto

Line 7 in 887d0b5

message Status {

Follow the status pattern used in the rest of coordinator.proto.

chroma/idl/chromadb/proto/coordinator.proto

Line 35 in 887d0b5

Status status = 2;

I don't think that is the correct pattern. I think we need to fix that code

Either we can:

Implement the correct change uniformly

Follow the existing pattern

Fragmentation of standards is not something we should be ok with rn.

I cannot fix everything I see. We should always be mindful about making incremental improvements. These improvements will come by priorities not by the fact that we see them. Also, fixing existing SysDB code is needed before we launch the product. I'm sure that we will fix it soon.

HammadB

gRPC service definitions should have error handling propagation.

Ishiihara · 2024-02-21T21:29:35Z

go/coordinator/internal/metastore/db/dao/record_log.go

 	"gorm.io/gorm"
+	"time"
 )

 type recordLogDb struct {
 	db *gorm.DB


We need to initialize db.

it is in earlier pr

Ishiihara · 2024-02-21T21:35:13Z

go/coordinator/internal/metastore/db/dao/record_log.go

+				Record:       &recordsContent[index],
+			})
+		}
+		err := tx.CreateInBatches(recordLogs, len(recordLogs)).Error


Just to confirm, this will commit if the transaction is successful.

test won't pass otherwise

Ishiihara · 2024-02-21T21:41:06Z

go/coordinator/internal/logservice/grpc/record_log_service_test.go

+		assert.Equal(t, collectionId.String(), *recordLogs[index].CollectionID)
+		record := &coordinatorpb.SubmitEmbeddingRecord{}
+		if err := proto.Unmarshal(*recordLogs[index].Record, record); err != nil {
+			panic(err)


Should not panic here?

Can we add a no-panic linter rule? Panic should be rare and we can expect ourselves to call its permissible use out explicitly.

even in tests?

I just mean orthogonally. Not in tests.

Agree, we should almost never panic. I would just assert.NotNil here.

Ishiihara · 2024-02-21T21:46:37Z

go/coordinator/internal/logservice/grpc/record_log_service.go

+			if err != nil {
+				return nil, grpcError
+			}
+			return nil, err


Should return a logservicepb.PushLogsResponse struct with proper field values.

I think we should return error if it's error case... otherwise your grpc respons will have 200 status code

I can go verify but I think the default interceptor is going to ignore the response anyways...

and if we are going to return results in error case, we should be very specific about what we are returning. is the result partial?

Ishiihara · 2024-02-21T21:46:53Z

go/coordinator/internal/logservice/grpc/record_log_service.go

+	recordCount, err := s.logService.PushLogs(ctx, collectionID, recordsContent)
+	if err != nil {
+		log.Error("error pushing logs", zap.Error(err))
+		return nil, grpcutils.BuildInternalGrpcError("error pushing logs")


Should return a logservicepb.PushLogsResponse struct with proper field values.

Ishiihara · 2024-02-21T21:48:32Z

idl/chromadb/proto/logservice.proto


+message PushLogsResponse {
+  int32 record_count = 1;


Should we also include a status code? Also, can you double check how GRPC handles golang errors in its response to client?

I'm confused... There is a Status in the grpc message itself why do we need to have a Status in the response? shouldn't it be the responsibility for the interceptor

I think it would be nice to have our own status on different errors for our service. For example, it is useful in SysDB to pass back to the front end why creating a collection is not successful so that the user can take the right action (e.g. unique constraint violation).

I am not sure how the python side handles the errors raised by the golang GRPC response.

My 2 cents, I think we could have both. I understand the value of having specific error code, but also It seems weird to have it in the response. I think we could use grpc response metadata to propagate the custom error code. Keeping native grpc code would help with monitoring and standard tooling. Putting it in metadata, allow us to set it at the framework lvl, and we don't have to add it in every response.

HammadB · 2024-02-21T22:48:48Z

idl/chromadb/proto/logservice.proto


+message PushLogsResponse {


Follow the status pattern used in the rest of coordinator.proto.

weiligu · 2024-02-22T02:50:08Z

gRPC service definitions should have error handling propagation.

@HammadB discussed offline. do you still want to request this change?

HammadB · 2024-02-22T03:00:56Z

gRPC service definitions should have error handling propagation.

@HammadB discussed offline. do you still want to request this change?

You said you were not doing it now - I don't follow what the point of this question is sorry. Do you want me to approve the PR?

weiligu · 2024-02-22T04:08:48Z

gRPC service definitions should have error handling propagation.

@HammadB discussed offline. do you still want to request this change?

You said you were not doing it now - I don't follow what the point of this question is sorry. Do you want me to approve the PR?

Do you still think this change should block this PR? Did we agree that this change can happen later in a different PR?

HammadB · 2024-02-22T04:14:58Z

gRPC service definitions should have error handling propagation.

@HammadB discussed offline. do you still want to request this change?

You said you were not doing it now - I don't follow what the point of this question is sorry. Do you want me to approve the PR?

Do you still think this change should block this PR? Did we agree that this change can happen later in a different PR?

No, we did not agree. But we can agree to disagree :)

weiligu · 2024-02-22T06:01:22Z

gRPC service definitions should have error handling propagation.

@HammadB discussed offline. do you still want to request this change?

You said you were not doing it now - I don't follow what the point of this question is sorry. Do you want me to approve the PR?

Do you still think this change should block this PR? Did we agree that this change can happen later in a different PR?

No, we did not agree. But we can agree to disagree :)

Fixing the SysDB gRPC is a separate non-trivial change and should not be included in this PR
I disagree with the other option to implement gRPC protocol in the wrong way just to make things consistent.
It is your call to disagree with above points and keep the change request.

HammadB · 2024-02-22T06:10:53Z

Sigh. I never said that we should do it in the same PR - I suggested to first figure out what we think the right way is with high confidence if how it is is wrong and then issue a stacked change so we don’t leave things halfway. Hammad Bashir EECS | Cal

…

On Wed, Feb 21, 2024 at 10:01 PM Weili Gu ***@***.***> wrote: gRPC service definitions should have error handling propagation. @HammadB <https://github.com/HammadB> discussed offline. do you still want to request this change? You said you were not doing it now - I don't follow what the point of this question is sorry. Do you want me to approve the PR? Do you still think this change should block this PR? Did we agree that this change can happen later in a different PR? No, we did not agree. But we can agree to disagree :) 1. Fixing the SysDB gRPC is a separate non-trivial change and should not be included in this PR 2. I disagree with the other option to implement gRPC protocol in the wrong way just to make things consistent. It is your call to disagree with above points and keep the change request. — Reply to this email directly, view it on GitHub <#1749 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKW32J34CA27ONUXQOWAZLYU3NL5AVCNFSM6AAAAABDTYDXOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYG43DCMRTGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

HammadB

shipit

HammadB · 2024-02-22T19:31:32Z

chromadb/logservice/grpc/client.py

+import grpc
+
+
+class GrpcLogService:


Suggestion to use the existing log abstractions or somehow rectify this with those.

weiligu · 2024-02-22T19:58:04Z

Sigh. I never said that we should do it in the same PR - I suggested to first figure out what we think the right way is with high confidence if how it is is wrong and then issue a stacked change so we don’t leave things halfway. Hammad Bashir EECS | Cal
…
On Wed, Feb 21, 2024 at 10:01 PM Weili Gu @.> wrote: gRPC service definitions should have error handling propagation. @HammadB https://github.com/HammadB discussed offline. do you still want to request this change? You said you were not doing it now - I don't follow what the point of this question is sorry. Do you want me to approve the PR? Do you still think this change should block this PR? Did we agree that this change can happen later in a different PR? No, we did not agree. But we can agree to disagree :) 1. Fixing the SysDB gRPC is a separate non-trivial change and should not be included in this PR 2. I disagree with the other option to implement gRPC protocol in the wrong way just to make things consistent. It is your call to disagree with above points and keep the change request. — Reply to this email directly, view it on GitHub <#1749 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKW32J34CA27ONUXQOWAZLYU3NL5AVCNFSM6AAAAABDTYDXOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYG43DCMRTGU . You are receiving this because you were mentioned.Message ID: @.>

Error handling for SysDB is covered in https://linear.app/trychroma/issue/CHR-281/sysdb-service-error-handling-and-retry-policy. I can add some notes in this ticket to also fix the Status Code. This does not seem to be a quick fix since we also need to fix front end services and I see this lower priority than getting Log service stand up and running. I agree that having two different implementations is not ideal but I also think we should accept incremental changes and work towards unified implementation.

nicolasgere · 2024-02-23T00:56:06Z

go/coordinator/internal/metastore/db/dao/record_log.go

+			zap.Int64("timestamp", timestamp),
+			zap.Int("count", len(recordsContent)))
+
+		var recordLogs []*dbmodel.RecordLog


you can make(type, size) if you know the size of the array to avoid append.

nicolasgere · 2024-02-23T00:59:19Z

go/coordinator/internal/logservice/grpc/record_log_service.go

+			log.Error("error building grpc error", zap.Error(err))
+			return nil, err
+		}
+		return nil, grpcError


I think we should use named return params. To avoid shadowing, and this kind of return

## Description of changes https://linear.app/trychroma/issue/CHR-295/push-log-api - PushLogs implementation in DAO - PushLogs API for gRPC - DB error handling and retry is not included ## Test plan - record_log_service test - record_log test

weiligu mentioned this pull request Feb 21, 2024

make collection_id primary key for segment, fix system tests #1748

Closed

1 task

weiligu changed the title ~~push logs with error handling, need to add arrow~~ push logs with error handling Feb 21, 2024

weiligu force-pushed the 02-20-push_logs branch from f39d158 to e0d8923 Compare February 21, 2024 21:04

weiligu changed the base branch from fix_system_test to main February 21, 2024 21:04

weiligu marked this pull request as ready for review February 21, 2024 21:09

weiligu requested a review from Ishiihara February 21, 2024 21:10

HammadB reviewed Feb 21, 2024

View reviewed changes

HammadB requested changes Feb 21, 2024

View reviewed changes

weiligu changed the title ~~push logs with error handling~~ push logs with grpc error handling Feb 21, 2024

Ishiihara reviewed Feb 21, 2024

View reviewed changes

HammadB changed the title ~~push logs with grpc error handling~~ [ENH] Push logs with grpc error handling Feb 21, 2024

HammadB requested changes Feb 21, 2024

View reviewed changes

idl/chromadb/proto/logservice.proto

message PushLogsResponse {

Copy link

Collaborator

HammadB Feb 21, 2024

Choose a reason for hiding this comment

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

Follow the status pattern used in the rest of coordinator.proto.

weiligu force-pushed the 02-20-push_logs branch from 1145424 to ca7b332 Compare February 22, 2024 02:29

HammadB approved these changes Feb 22, 2024

View reviewed changes

weiligu added 7 commits February 22, 2024 14:24

push logs with error handling, need to add arrow

9469dac

push logs

f2a7db4

clean up

71eca2c

clean up mod

83cd317

error

55bafa8

push logs with error handling, need to add arrow

78d3e61

push logs

188941a

weiligu added 9 commits February 22, 2024 14:29

clean up mod

272cd1f

error

ec4ce45

add logservice python tests

6518f4c

add skip local test flag

3c49cc7

rename for hypo test conflicts

9aaef21

try removing tests

26906f5

log service python tests

7e54f25

remove python testing code

88a1244

fix proto

b3e082c

weiligu force-pushed the 02-20-push_logs branch from eefa7d8 to b3e082c Compare February 22, 2024 22:31

weiligu mentioned this pull request Feb 23, 2024

[ENH] Server side pull logs #1762

Merged

2 tasks

nicolasgere reviewed Feb 23, 2024

View reviewed changes

weiligu merged commit e61ac08 into main Feb 23, 2024
117 checks passed

		import grpc


		class GrpcLogService:

[ENH] Push logs with grpc error handling #1749

[ENH] Push logs with grpc error handling #1749

Conversation

weiligu commented Feb 21, 2024 • edited

Description of changes

Test plan

weiligu commented Feb 21, 2024 • edited

github-actions bot commented Feb 21, 2024

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HammadB Feb 21, 2024 • edited

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

Choose a reason for hiding this comment

HammadB 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

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

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

nicolasgere Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiligu commented Feb 22, 2024 • edited

HammadB commented Feb 22, 2024 • edited

weiligu commented Feb 22, 2024 • edited

HammadB commented Feb 22, 2024

weiligu commented Feb 22, 2024

HammadB commented Feb 22, 2024 via email

HammadB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiligu commented Feb 22, 2024

nicolasgere Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiligu commented Feb 21, 2024 •

edited

weiligu commented Feb 21, 2024 •

edited

HammadB Feb 21, 2024 •

edited

nicolasgere Feb 23, 2024 •

edited

weiligu commented Feb 22, 2024 •

edited

HammadB commented Feb 22, 2024 •

edited

weiligu commented Feb 22, 2024 •

edited

nicolasgere Feb 23, 2024 •

edited