Remove deprecated Query Server flags #2772

rjs211 · 2021-02-03T03:14:12Z

Signed-off-by: rjs211 srivatsa211@gmail.com

Which problem is this PR solving?

resolves part of Remove deprecated CLI flags #2738

Short description of the changes

The deprecated query.port and query.host-port were removed.
The redundant test cases were removed
Even though the behaviour remaiins the same, the code logic is simplified.

Signed-off-by: rjs211 <srivatsa211@gmail.com>

rjs211 · 2021-02-03T03:17:42Z

The new logic is as follows:

If TLS in enabled in either or both of GRPC or HTTP endpoints, --query.http-server.host-portdefaults to :16686 and --query.grpc-server.host-port defaults to :16685
If TLS is disabled in both endpoints (default), both --query.http-server.host-port and --query.grpc-server.host-port default to :16686

This ensures that if the user doesn't specify the host-ports, the server would still initialize, for both TLS and non-TLS cases, conforming to the existing behaviour.

codecov · 2021-02-03T03:26:15Z

Codecov Report

Merging #2772 (5421036) into master (9978f02) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2772      +/-   ##
==========================================
- Coverage   95.91%   95.89%   -0.02%     
==========================================
  Files         217      217              
  Lines        9634     9619      -15     
==========================================
- Hits         9240     9224      -16     
  Misses        326      326              
- Partials       68       69       +1

Impacted Files	Coverage Δ
cmd/query/app/flags.go	`100.00% <ø> (ø)`
cmd/query/app/server.go	`97.08% <100.00%> (-0.05%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`95.37% <0.00%> (-0.72%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9978f02...5421036. Read the comment docs.

yurishkuro

Please add an entry to the changelog (and correct the previous one if applicable)

yurishkuro · 2021-02-03T03:28:13Z

ports/ports.go

@@ -38,7 +38,8 @@ const (
 	// CollectorAdminHTTP is the default admin HTTP port (health check, metrics, etc.)
 	CollectorAdminHTTP = 14269

-	// QueryGRPC is the default port of GRPC requests for Query trace retrieval
+	// QueryGRPC is the default port of GRPC requests for Query trace retrieval when TLS is enabled.
+	// When TLS is disabled, QueryHTTP is used for both GRPC and HTTP endpoints


Suggested change

// When TLS is disabled, QueryHTTP is used for both GRPC and HTTP endpoints

// When TLS is disabled and GRPC port not provided, QueryHTTP is used for both GRPC and HTTP endpoints

Signed-off-by: rjs211 <srivatsa211@gmail.com>

… dev-rmQryFlgs

yurishkuro · 2021-02-03T03:57:46Z

CHANGELOG.md

@@ -8,6 +8,8 @@ Changes by Version

 #### Breaking Changes

+* Remove deprecated flags of Query Server  `--query.port` and `--query.host-port`, please use dedicated HTTP `--query.http-server.host-port` and GRPC `--query.grpc-server.host-port`  host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))


Suggested change

* Remove deprecated flags of Query Server `--query.port` and `--query.host-port`, please use dedicated HTTP `--query.http-server.host-port` and GRPC `--query.grpc-server.host-port` host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))

* Remove deprecated Query Server flags `--query.port` and `--query.host-port`, please use dedicated `--query.http-server.host-port` and `--query.grpc-server.host-port` flags ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))

yurishkuro · 2021-02-03T04:03:40Z

CHANGELOG.md


-    If TLS is disabled in both endpoints (default), dedicated host-ports are used if either or both of `--query.http-server.host-port` or `--query.grpc-server.host-port` is explicitly set. If only one of the dedicated host-port flags is set, the other flag uses its corresponding default value. If neither of the dedicated host-port flags are explicitly set, the common host-port infered from `--query.host-port` (defaults to `:16686`) is used for both the GRPC and HTTP endpoints.
+  *  If TLS in enabled in either or both of GRPC or HTTP endpoints, `--query.http-server.host-port`defaults to `:16686` and  `--query.grpc-server.host-port` defaults to `:16685`  
+  *  If TLS is disabled in both endpoints (default), both `--query.http-server.host-port` and  `--query.grpc-server.host-port` default to `:16686`  


I am a bit concerned that this will be confusing. Both new flags have default values (different ones), so the traditional behavior of the program when a flag is not specified explicitly is to use the default. However, here we're saying that even though the help command will show :16685 as default, the actual server is going to run on the shared :16686 port.

Perhaps we could just move this into breaking changes section and always treat the ports as separate, unless the same value is given in the config (which should only work for non-TLS).

@jpkrohling thoughts?

Perhaps we could just move this into breaking changes section and always treat the ports as separate, unless the same value is given in the config (which should only work for non-TLS).

If we are breaking, then I'd force people to always use a different port. It makes the code easier to maintain, and I believe this is a good practice anyway...

I think that would be too "breaking". It's one thing to to change the flags, it's another to completely remove the ability to run on the same port. The same-port CMUX approach seems to work fine for non-TLS, I would rather keep it.

Sounds good to me.

Signed-off-by: rjs211 <srivatsa211@gmail.com>

… dev-rmQryFlgs

yurishkuro

THanks!

rjs211 added 2 commits February 3, 2021 08:25

Removing depricated query server flags

8e1ad3f

Signed-off-by: rjs211 <srivatsa211@gmail.com>

modify comments to make things more clear

64c0e25

Signed-off-by: rjs211 <srivatsa211@gmail.com>

rjs211 requested a review from a team as a code owner February 3, 2021 03:14

rjs211 requested a review from objectiser February 3, 2021 03:14

mergify bot assigned jpkrohling Feb 3, 2021

mergify bot requested a review from jpkrohling February 3, 2021 03:14

yurishkuro changed the title ~~Removing depricated Query Server flags~~ Removing deprecated Query Server flags Feb 3, 2021

yurishkuro reviewed Feb 3, 2021

View reviewed changes

rjs211 added 2 commits February 3, 2021 09:18

adding changelog, correcting comments

9f24b16

Signed-off-by: rjs211 <srivatsa211@gmail.com>

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

03ed617

… dev-rmQryFlgs

yurishkuro reviewed Feb 3, 2021

View reviewed changes

rjs211 added 5 commits February 4, 2021 06:46

changing default behaviour of host-port flags

d58c20c

Signed-off-by: rjs211 <srivatsa211@gmail.com>

pruning test cases, renaming test method

0633da0

Signed-off-by: rjs211 <srivatsa211@gmail.com>

changing comments for host-port default

9ff874f

Signed-off-by: rjs211 <srivatsa211@gmail.com>

changelog for default query server behaviour change

0c058fa

Signed-off-by: rjs211 <srivatsa211@gmail.com>

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

5421036

… dev-rmQryFlgs

rjs211 requested a review from yurishkuro February 4, 2021 01:48

yurishkuro approved these changes Feb 4, 2021

View reviewed changes

yurishkuro merged commit 1e1d244 into jaegertracing:master Feb 4, 2021

yurishkuro changed the title ~~Removing deprecated Query Server flags~~ Remove deprecated Query Server flags Feb 4, 2021

yurishkuro mentioned this pull request Feb 4, 2021

Remove deprecated CLI flags #2738

Closed

7 tasks

albertteoh mentioned this pull request Feb 23, 2021

Preparing release v1.22.0 #2826

Merged

dependabot bot mentioned this pull request Mar 8, 2021

Bump github.com/jaegertracing/jaeger from 1.21.0 to 1.22.0 httpsgithu/opentelemetry-collector#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated Query Server flags #2772

Remove deprecated Query Server flags #2772

rjs211 commented Feb 3, 2021 •

edited

Loading

rjs211 commented Feb 3, 2021

codecov bot commented Feb 3, 2021 •

edited

Loading

yurishkuro left a comment

yurishkuro Feb 3, 2021

yurishkuro Feb 3, 2021

yurishkuro Feb 3, 2021

jpkrohling Feb 3, 2021

yurishkuro Feb 3, 2021

jpkrohling Feb 3, 2021

yurishkuro left a comment

	// When TLS is disabled, QueryHTTP is used for both GRPC and HTTP endpoints
	// When TLS is disabled and GRPC port not provided, QueryHTTP is used for both GRPC and HTTP endpoints

		@@ -8,6 +8,8 @@ Changes by Version

		#### Breaking Changes

		* Remove deprecated flags of Query Server `--query.port` and `--query.host-port`, please use dedicated HTTP `--query.http-server.host-port` and GRPC `--query.grpc-server.host-port` host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))

	* Remove deprecated flags of Query Server `--query.port` and `--query.host-port`, please use dedicated HTTP `--query.http-server.host-port` and GRPC `--query.grpc-server.host-port` host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))
	* Remove deprecated Query Server flags `--query.port` and `--query.host-port`, please use dedicated `--query.http-server.host-port` and `--query.grpc-server.host-port` flags ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))

Remove deprecated Query Server flags #2772

Remove deprecated Query Server flags #2772

Conversation

rjs211 commented Feb 3, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

rjs211 commented Feb 3, 2021

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Feb 3, 2021

Choose a reason for hiding this comment

yurishkuro Feb 3, 2021

Choose a reason for hiding this comment

yurishkuro Feb 3, 2021

Choose a reason for hiding this comment

jpkrohling Feb 3, 2021

Choose a reason for hiding this comment

yurishkuro Feb 3, 2021

Choose a reason for hiding this comment

jpkrohling Feb 3, 2021

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

rjs211 commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading