[BUG]: increase max payload size of log service (Go) #4534
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
This PR fixes a bug where the log service was still limited to 4MB payload size, despite the Rust log client being updated to handle larger payloads. The change uses the shared gRPC utility with proper payload size configuration. This summary was automatically generated by @propel-code-bot |
HammadB
reviewed
May 12, 2025
| @@ -158,7 +158,7 @@ func NewWithGrpcProvider(config Config, provider grpcutils.GrpcProvider) (*Serve | |||
| s.softDeleteCleaner.Start() | |||
|
|
|||
| log.Info("Starting GRPC server") | |||
| s.grpcServer, err = provider.StartGrpcServer("coordinator", config.GrpcConfig, func(registrar grpc.ServiceRegistrar) { | |||
| s.grpcServer, err = provider.StartGrpcServer("sysdb-service", config.GrpcConfig, func(registrar grpc.ServiceRegistrar) { | |||
There was a problem hiding this comment.
why this change? does anything downstream for telemetry use it are we sure nothing depends on it?
There was a problem hiding this comment.
the coordinator value was not previously used because sysdb-service was hard coded in the common helper
HammadB
approved these changes
May 12, 2025
rescrv
reviewed
May 14, 2025
| var listener net.Listener | ||
| listener, err = net.Listen("tcp", ":"+config.PORT) | ||
|
|
||
| _, err = grpcutils.Default.StartGrpcServer("log-service", &grpcutils.GrpcConfig{ |
There was a problem hiding this comment.
This line loses sharedOtel?
There was a problem hiding this comment.
inside of the grpcutils package, it's using the shared otel package (, so I think this should be equivalent, but I might be missing something.
eculver
added a commit
that referenced
this pull request
May 14, 2025
## Description of changes This restores the core functionality we were hoping to add in #4534 which is increasing the gRPC payload size. Instead of using the methods from the `grpcutils` package, this just sets the option in the existing setup so that we hopefully don't break whatever voodoo is keeping metrics collection working. - Improvements & Bug fixes - See #4534 - New functionality - See #4534 ## Test plan I ran `make test` locally, the build succeeds, but our local tests are broken for some reason. Relying on CI so that I don't have to debug yet another orthogonal issue.
itaismith
pushed a commit
that referenced
this pull request
May 23, 2025
The max gRPC payload/response sizes were updated for the Rust log client, but were never updated for the log service. So the `PushLogs` method was still failing when the request was > 4MB. _How are these changes tested?_ - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust _Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs section](https://github.com/chroma-core/chroma/tree/main/docs/docs.trychroma.com)?_ n/a checkpoint after adding default checkpoint test passing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
The max gRPC payload/response sizes were updated for the Rust log client, but were never updated for the log service. So the
PushLogsmethod was still failing when the request was > 4MB.Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?
n/a