Skip to content

Commit 737794a

Browse files
committed
[feat/1316] test cases fix
1 parent fa06d46 commit 737794a

File tree

4 files changed

+179
-14
lines changed

4 files changed

+179
-14
lines changed

jupyter_server/prometheus/server.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,6 @@ def start(self, port: int) -> None:
198198
# Last attempt failed
199199
self.server_app.log.warning(
200200
f"Could not start metrics server on any port after {max_retries} attempts. "
201-
f"Metrics will be available on the main server at /metrics"
202201
)
203202
return
204203
else:

jupyter_server/serverapp.py

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2836,6 +2836,11 @@ def initialize(
28362836
self.init_mime_overrides()
28372837
self.init_shutdown_no_activity()
28382838
self.init_metrics()
2839+
2840+
# Start metrics server after webapp is initialized, so handlers can be properly excluded
2841+
if self.metrics_port:
2842+
self._start_metrics_server(self.metrics_port)
2843+
28392844
if new_httpserver:
28402845
self.init_httpserver()
28412846

@@ -2878,9 +2883,15 @@ def running_server_info(self, kernel_count: bool = True) -> str:
28782883
version=ServerApp.version, url=self.display_url
28792884
)
28802885
info += "\n"
2886+
# Show metrics URL - if metrics_port is set, the separate server is guaranteed to be running
28812887
if self.metrics_port:
28822888
info += _i18n("Metrics server is running at:\n{url}").format(
2883-
url=f"http://localhost:{self.metrics_port}/metrics"
2889+
url=f"http://localhost:{self.metrics_server.port}/metrics"
2890+
)
2891+
info += "\n"
2892+
else:
2893+
info += _i18n("Metrics are available at:\n{url}").format(
2894+
url=f"{self.connection_url.rstrip('/')}/metrics"
28842895
)
28852896
info += "\n"
28862897
if self.gateway_config.gateway_enabled:
@@ -3054,15 +3065,13 @@ def _start_metrics_server(self, port):
30543065
self.metrics_server = start_metrics_server(self, port)
30553066
# Check if the metrics server actually started (has a port)
30563067
if not hasattr(self.metrics_server, "port") or self.metrics_server.port is None:
3057-
self.metrics_server = None
3058-
self.log.warning(
3059-
"Metrics server failed to start. Metrics will be available on the main server at /metrics"
3060-
)
3068+
raise RuntimeError("Metrics server failed to start - no port assigned")
3069+
3070+
self.log.info(f"Metrics server is running on port {self.metrics_server.port}")
3071+
30613072
except Exception as e:
3062-
self.metrics_server = None
3063-
self.log.warning(
3064-
f"Failed to start metrics server: {e}. Metrics will be available on the main server at /metrics"
3065-
)
3073+
self.log.error(f"Failed to start metrics server: {e}")
3074+
raise RuntimeError(f"Metrics server is required but failed to start: {e}")
30663075

30673076
def launch_browser(self) -> None:
30683077
"""Launch the browser."""
@@ -3130,10 +3139,6 @@ def start_app(self) -> None:
31303139
if self.open_browser and not self.sock:
31313140
self.launch_browser()
31323141

3133-
# Start metrics server on alternate port if configured
3134-
if self.metrics_port:
3135-
self._start_metrics_server(self.metrics_port)
3136-
31373142
if self.identity_provider.token and self.identity_provider.token_generated:
31383143
# log full URL with generated token, so there's a copy/pasteable link
31393144
# with auth info.

tests/conftest.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import os
22

3+
# Disable metrics server for all tests by default
4+
os.environ["JUPYTER_SERVER_METRICS_PORT"] = "0"
5+
36
# isort: off
47
# This must come before any Jupyter imports.
58
os.environ["JUPYTER_PLATFORM_DIRS"] = "1"

tests/test_metrics.py

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
"""Tests for Jupyter Server metrics functionality."""
2+
3+
import pytest
4+
import requests
5+
import time
6+
from unittest.mock import patch
7+
8+
from jupyter_server.prometheus.server import PrometheusMetricsServer, start_metrics_server
9+
from jupyter_server.serverapp import ServerApp
10+
11+
12+
@pytest.fixture
13+
def metrics_server_app():
14+
"""Create a server app with metrics enabled on a specific port."""
15+
# Override the environment variable for this test
16+
with patch.dict('os.environ', {'JUPYTER_SERVER_METRICS_PORT': '9090'}):
17+
app = ServerApp()
18+
# Set the metrics_port directly as a trait
19+
app.metrics_port = 9090
20+
app.initialize([])
21+
return app
22+
23+
24+
@pytest.fixture
25+
def metrics_server(metrics_server_app):
26+
"""Start a metrics server for testing."""
27+
server = start_metrics_server(metrics_server_app, 9090)
28+
# Give the server time to start
29+
time.sleep(0.1)
30+
yield server
31+
# Cleanup
32+
if hasattr(server, 'stop'):
33+
server.stop()
34+
35+
36+
def test_metrics_server_starts(metrics_server):
37+
"""Test that the metrics server starts successfully."""
38+
assert metrics_server is not None
39+
assert hasattr(metrics_server, 'port')
40+
assert metrics_server.port == 9090
41+
42+
43+
def test_metrics_endpoint_accessible(metrics_server):
44+
"""Test that the metrics endpoint is accessible."""
45+
response = requests.get(f'http://localhost:{metrics_server.port}/metrics')
46+
assert response.status_code == 200
47+
assert 'jupyter_server' in response.text
48+
49+
50+
def test_metrics_contains_kernel_metrics(metrics_server):
51+
"""Test that kernel metrics are present."""
52+
response = requests.get(f'http://localhost:{metrics_server.port}/metrics')
53+
assert response.status_code == 200
54+
content = response.text
55+
assert 'jupyter_kernel_currently_running_total' in content
56+
57+
58+
def test_metrics_contains_server_info(metrics_server):
59+
"""Test that server info metrics are present."""
60+
response = requests.get(f'http://localhost:{metrics_server.port}/metrics')
61+
assert response.status_code == 200
62+
content = response.text
63+
assert 'jupyter_server_info' in content
64+
65+
66+
def test_metrics_server_with_authentication():
67+
"""Test metrics server with authentication enabled."""
68+
app = ServerApp()
69+
app.metrics_port = 9091
70+
app.authenticate_prometheus = True
71+
app.initialize([])
72+
app.identity_provider.token = "test_token"
73+
74+
server = start_metrics_server(app, 9091)
75+
time.sleep(0.1)
76+
77+
try:
78+
# Without token should fail
79+
response = requests.get(f'http://localhost:{server.port}/metrics')
80+
assert response.status_code == 401
81+
82+
# With token should succeed
83+
response = requests.get(f'http://localhost:{server.port}/metrics?token=test_token')
84+
assert response.status_code == 200
85+
finally:
86+
if hasattr(server, 'stop'):
87+
server.stop()
88+
89+
90+
def test_metrics_server_port_conflict_handling():
91+
"""Test that metrics server handles port conflicts gracefully."""
92+
app = ServerApp()
93+
app.metrics_port = 9092
94+
app.initialize([])
95+
server2 = None
96+
# Start first server
97+
server1 = start_metrics_server(app, 9092)
98+
time.sleep(0.1)
99+
100+
try:
101+
# Try to start second server on same port
102+
server2 = start_metrics_server(app, 9092)
103+
time.sleep(0.1)
104+
105+
# One of them should have failed to start or used a different port
106+
if server2 is not None and hasattr(server2, 'port'):
107+
assert server2.port != 9092 or server1.port != 9092
108+
finally:
109+
if hasattr(server1, 'stop'):
110+
server1.stop()
111+
if server2 is not None and hasattr(server2, 'stop'):
112+
server2.stop()
113+
114+
115+
def test_metrics_server_disabled_when_port_zero():
116+
"""Test that metrics server is not started when port is 0."""
117+
with patch.dict('os.environ', {'JUPYTER_SERVER_METRICS_PORT': '0'}):
118+
app = ServerApp()
119+
app.metrics_port = 0
120+
app.initialize([])
121+
122+
# Should not start metrics server
123+
assert not hasattr(app, 'metrics_server') or app.metrics_server is None
124+
125+
126+
def test_metrics_url_logging_with_separate_server():
127+
"""Test that metrics URL is logged correctly with separate server."""
128+
app = ServerApp()
129+
app.metrics_port = 9093
130+
app.authenticate_prometheus = True
131+
app.initialize([])
132+
app.identity_provider.token = "test_token"
133+
134+
# Start metrics server
135+
server = start_metrics_server(app, 9093)
136+
time.sleep(0.1)
137+
138+
try:
139+
# The URL should include the separate port
140+
expected_url = "http://localhost:9093/metrics?token=test_token"
141+
# This is a basic test - in practice you'd capture the log output
142+
assert server.port == 9093
143+
finally:
144+
if hasattr(server, 'stop'):
145+
server.stop()
146+
147+
148+
def test_metrics_url_logging_with_main_server():
149+
"""Test that metrics URL is logged correctly when using main server."""
150+
app = ServerApp()
151+
app.metrics_port = 0 # Disable separate server
152+
app.authenticate_prometheus = True
153+
app.initialize([])
154+
app.identity_provider.token = "test_token"
155+
156+
# Should use main server's /metrics endpoint
157+
# This would be tested by checking the log output in practice
158+
assert app.metrics_port == 0

0 commit comments

Comments
 (0)