Admin/Query: Log the real port instead of the provided one to enable the use of port 0 #2002

ChadiEM · 2020-01-06T15:22:08Z

Which problem is this PR solving?

When using port 0 with the admin or query port, the Jaeger components print 0 instead of the port actually used. What this PR changes is to print the actual port.

Short description of the changes

Uses net.SplitHostPort to get the actual port.

yurishkuro

nvm

cmd/query/app/server_test.go

yurishkuro · 2020-01-06T16:03:35Z

cmd/query/app/server_test.go

+	onlyEntry := message.All()[0]
+	port := onlyEntry.ContextMap()["port"].(int)
+	assert.True(t, port > 0,
+		"Expected a non-zero port in the logs, got instead: %d", port)


strictly speaking, it is possible that NewServer() function replaces 0 port with the default value, in which case this test will not catch it.

one way to guarantee that it does is by starting two servers and validating that they both use different ports.

can we fix this?

strictly speaking, it is possible that NewServer() function replaces 0 port with the default value, in which case this test will not catch it.

one way to guarantee that it does is by starting two servers and validating that they both use different ports.

Sure, I will add that test. However, what this PR targets is printing the real port in the log file instead of 0 and not the fact of using port 0 itself.
Using 0 works even before this PR :-)

btw, I also have a gap for testing the admin port log output. In the current state, I do not have a way to set the logger on the AdminServer. What I can do is to expose setLogger() and use it, then call Serve() and check the log in that case. Would that be ok?

Logger can be passed via InitFromViper. Look for Viperize helper function somewhere in the repo.

I added tests for launching two instances in parallel. I used a cyclicbarrier to make sure that both servers are synchronized after the Start(), because without it, each goroutine can run on its own and the test behavior will be random. Let me know if you are ok with the thirdparty, or if you have other suggestions. :-)

pkg/netutils/port.go

codecov · 2020-01-07T01:02:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@162eee0). Click here to learn what that means.
The diff coverage is 93.75%.

@@           Coverage Diff            @@
##             master   #2002   +/-   ##
========================================
  Coverage          ?   97.4%           
========================================
  Files             ?     206           
  Lines             ?   10136           
  Branches          ?       0           
========================================
  Hits              ?    9873           
  Misses            ?     221           
  Partials          ?      42

Impacted Files	Coverage Δ
pkg/queue/bounded_queue.go	`94.8% <100%> (ø)`
pkg/discovery/grpcresolver/grpc_resolver.go	`98.57% <50%> (ø)`

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 162eee0...00b01f4. Read the comment docs.

pkg/netutils/port_test.go

yurishkuro · 2020-01-07T03:09:52Z

cmd/query/app/server_test.go

+	onlyEntry := message.All()[0]
+	port := onlyEntry.ContextMap()["port"].(int)
+	assert.True(t, port > 0,
+		"Expected a non-zero port in the logs, got instead: %d", port)


can we fix this?

cmd/query/app/server_test.go

…the use of port 0. Signed-off-by: Chadi El Masri <[email protected]>

Signed-off-by: Chadi El Masri <[email protected]>

ChadiEM · 2020-01-09T10:39:48Z

For consistency, I could also apply this change to the agent code (although it uses a different method to get the port with an equivalent result).

ChadiEM · 2020-01-10T17:38:44Z

Is the codecov/patch failure due to the lack of coverage of the error cases in the GetPort function?

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro

to avoid another roundtrip I applied the changes

yurishkuro · 2020-01-11T02:13:32Z

cmd/flags/admin_test.go

+	assert.Greater(t, port, int64(0))
+}
+
+func TestAdminServerHandlesSimultaneousStartupOnPortZero(t *testing.T) {


I don't think this test is necessary (two admin servers is not a real use case). In fact, my comment about two servers was not to test two servers themselves, but to test that when port 0 is passed as a parameter it's not overridden by some fixed default port number, since this is what was the reason for your PR (iirc).

yurishkuro · 2020-01-11T02:14:57Z

cmd/flags/admin_test.go

+				defer adminServer.Close()
+			}
+
+			require.NoError(t, barrier.Await(context.Background()))


what are trying to achieve with this?

yurishkuro · 2020-01-11T02:47:51Z

Thanks for the PR!

ChadiEM · 2020-01-12T15:05:34Z

Thanks @yurishkuro!

ChadiEM requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners January 6, 2020 15:22

yurishkuro reviewed Jan 6, 2020

View reviewed changes

yurishkuro reviewed Jan 7, 2020

View reviewed changes

pkg/netutils/port.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 7, 2020

View reviewed changes

ChadiEM added 8 commits January 8, 2020 12:50

Admin/Query: Log the real port instead of the provided one to enable …

06f482e

…the use of port 0. Signed-off-by: Chadi El Masri <[email protected]>

Added comment on GetPort.

6b99d1a

Signed-off-by: Chadi El Masri <[email protected]>

Use assert.Greater.

6327582

Signed-off-by: Chadi El Masri <[email protected]>

End comment with a period.

0fe8c88

Signed-off-by: Chadi El Masri <[email protected]>

Added unit test for port.

7883c47

Signed-off-by: Chadi El Masri <[email protected]>

Removed redundant error message.

77d0b5e

Signed-off-by: Chadi El Masri <[email protected]>

Added unit test for admin.

74f53b2

Signed-off-by: Chadi El Masri <[email protected]>

Added tests to launch two servers at port 0.

ed3812f

Signed-off-by: Chadi El Masri <[email protected]>

Yuri Shkuro added 2 commits January 10, 2020 21:26

Remove unnecessary tests and 3rd party deps

162eee0

Signed-off-by: Yuri Shkuro <[email protected]>

Merge branch 'master' into print-real-port-number

00b01f4

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Jan 11, 2020

View reviewed changes

yurishkuro merged commit 3becf52 into jaegertracing:master Jan 11, 2020

ChadiEM deleted the print-real-port-number branch January 12, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin/Query: Log the real port instead of the provided one to enable the use of port 0 #2002

Admin/Query: Log the real port instead of the provided one to enable the use of port 0 #2002

ChadiEM commented Jan 6, 2020 •

edited

Loading

yurishkuro left a comment •

edited

Loading

yurishkuro Jan 6, 2020

yurishkuro Jan 6, 2020

yurishkuro Jan 7, 2020

ChadiEM Jan 7, 2020

yurishkuro Jan 7, 2020

ChadiEM Jan 8, 2020

codecov bot commented Jan 7, 2020 •

edited

Loading

yurishkuro Jan 7, 2020

ChadiEM commented Jan 9, 2020

ChadiEM commented Jan 10, 2020

yurishkuro left a comment

yurishkuro Jan 11, 2020

yurishkuro Jan 11, 2020

yurishkuro commented Jan 11, 2020

ChadiEM commented Jan 12, 2020

Admin/Query: Log the real port instead of the provided one to enable the use of port 0 #2002

Admin/Query: Log the real port instead of the provided one to enable the use of port 0 #2002

Conversation

ChadiEM commented Jan 6, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro left a comment • edited Loading

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

codecov bot commented Jan 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ChadiEM commented Jan 9, 2020

ChadiEM commented Jan 10, 2020

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 11, 2020

ChadiEM commented Jan 12, 2020

ChadiEM commented Jan 6, 2020 •

edited

Loading

yurishkuro left a comment •

edited

Loading

codecov bot commented Jan 7, 2020 •

edited

Loading