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

Redundant flag --reporter.type=grpc when used along with --reporter.grpc.host-port? #1376

Closed
annanay25 opened this issue Feb 25, 2019 · 5 comments

Comments

@annanay25
Copy link
Member

Requirement - what kind of business use case are you trying to solve?

Make agent config flags more intuitive.

Problem - what in Jaeger blocks you from solving the requirement?

Jaeger agent deployment as of now requires the use of --reporter.type=grpc --reporter.grpc.host-port=host:port to report to collector using grpc.

Proposal - what do you suggest to solve the problem or improve the existing situation?

Remove the redundant flag as passing --reporter.grpc.host-port=host:port is sufficient to specify GRPC usage; similar to how --reporter.tchannel.host-port=host:port is sufficient to specify tchannel use.

Any open questions to address

_

@pavolloffay
Copy link
Member

similar to how --reporter.tchannel.host-port=host:port is sufficient to specify tchannel use.

this is because of backward compatibility. Before we had only one reporter - tchannel. We will be changing default reporter to grpc in the next release #1218.

@annanay25
Copy link
Member Author

Ah, that makes sense @pavolloffay

We could in the next release, check for the presence of either --reporter.tchannel.host-port or --reporter.grpc.host-port and decide on the reporter.type without having the user explicitly pass the flag.

We could mark this a "good first issue".

@yurishkuro
Copy link
Member

I prefer to keep the selector flag until we remove tchannel completely, because if we only rely on the presence of the host-port flag, we have an ambiguity when it's provided for both transports, and we'd need to default to one (or crash, which is perhaps better).

@pavolloffay
Copy link
Member

@annanay25 I will close this for now. We also don't want to remove flags as it causes a breaking change so this could happen soonest with the next major version.

@annanay25
Copy link
Member Author

Sure, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants