Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to configure max received message size for jaeger-query #4351

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SOF3
Copy link

@SOF3 SOF3 commented Mar 29, 2023

Currently this option only exists for collector but not for query.

This option is required when a storage returns a very large grpc message (>4MiB).

Which problem is this PR solving?

Short description of the changes

  • Add a --grpc-storage.max-recv-msg-size option.

@SOF3 SOF3 force-pushed the query-max-grpc-receive-length branch from 2f6df6e to 9d94b7d Compare March 29, 2023 08:54
@SOF3 SOF3 marked this pull request as ready for review March 29, 2023 08:55
@SOF3 SOF3 requested a review from a team as a code owner March 29, 2023 08:55
@SOF3 SOF3 requested a review from joe-elliott March 29, 2023 08:55
@ChillOrb
Copy link
Contributor

@SOF3 What if someone wants to set it as -1 (no limit) ref?

@SOF3
Copy link
Author

SOF3 commented Mar 30, 2023

@ChillOrb the argument interpretation is copied from the equivalent option in collector --collector.grpc-server.max-message-size. It is also > 0 there.

@ChillOrb
Copy link
Contributor

ChillOrb commented Mar 31, 2023

Looking at this a bit more, I found that currently grpc-go only supports MaxRecvMsgSize and not something like MaxRecvMsgLength(which is a valid option link) so for linkthe condition > 0 makes sense.

As for the following variable nameflagSuffixGRPCMaxReceiveMessageLength which is accepted from "max-message-size" link I feel this should instead be called flagSuffixGRPCMaxReceiveMessageSize since MaxMessageLength has a different meaning in grpc terms and can be set as -1 and thus should not have > 0 condition.

@yurishkuro What do you think?

@SOF3 What if someone wants to set it as -1 (no limit) ref?

@yurishkuro
Copy link
Member

Go API calls is "size", which is what we call it too. "length" is from some other documentation.

@yurishkuro
Copy link
Member

any follow-up?

@SOF3 SOF3 force-pushed the query-max-grpc-receive-length branch from 9d94b7d to 8da3ed8 Compare April 18, 2023 01:59
cmd/query/app/server.go Outdated Show resolved Hide resolved
@SOF3 SOF3 force-pushed the query-max-grpc-receive-length branch from 8da3ed8 to 6896a5b Compare May 9, 2023 10:26
…rage plugin

Signed-off-by: Chan Kwan Yin <chankyin@bytedance.com>
@SOF3 SOF3 force-pushed the query-max-grpc-receive-length branch from 6896a5b to 218f9cc Compare May 9, 2023 10:27
@SOF3
Copy link
Author

SOF3 commented May 9, 2023

Rewrote the commit, changing the grpc storage instad of the query command.

@@ -73,5 +75,6 @@ func (opt *Options) InitFromViper(v *viper.Viper) error {
}
opt.Configuration.RemoteConnectTimeout = v.GetDuration(remoteConnectionTimeout)
opt.Configuration.TenancyOpts = tenancy.InitFromViper(v)
opt.Configuration.MaxRecvMsgSize = v.GetInt(grpcMaxRecvMsgSize)
Copy link
Member

Choose a reason for hiding this comment

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

please add to tests

plugin/storage/grpc/options.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants