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.

Sign up for GitHub

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

Jump to bottom

Add a traitlet to disable recording HTTP request metrics #1472

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Add a traitlet to disable recording HTTP request metrics #1472

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9508c5b
Add a traitlet to disable recording HTTP request metrics
yuvipanda Nov 4, 2024
ba416b9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 4, 2024
a4cf579
Fix typo
yuvipanda Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions jupyter_server/log.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,16 @@ def _scrub_uri(uri: str) -> str:
return uri


def log_request(handler):
def log_request(handler, record_prometheus_metrics=True):
Copy link
Member

Choose a reason for hiding this comment

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

Expanding the signature here makes me a little more cautious to merge. This technically expands a public API which would normally be saved for a major release. However, we rarely make a major release of Jupyter Server (because it requires a lot of work to coordinate with out subprojects).

On the other hand, this change seems small enough that it likely shouldn't trigger a major release. We could also argue that this API is likely unused by anyone outside Jupyter Server, since it's really specific to Jupyter Server. The only people this might affect are folks that monkeypatch this method, which is discouraged.

I think we can proceed as long as we communicate clearly that there's a "possible breaking change" when this is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw, that's why I made it a default arg and set it to the value that would cause no behavior change when omitted! So a log_request(handler) from any other project would see no difference than before.

Copy link
Member

@Zsailer Zsailer Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Totally, and this is great!

The specific case I was thinking about was where someone patches log_request to intercept our logger. Something like this:

import jupyter_server.log

# Create a custom function to monkeypatch jupyter server's log_request 
def log_request(handler):
    ...
    # custom logic

jupyter_server.log.log_request = log_request

A Jupyter Server using this monkeypatch would fail after releasing this PR right?

This is definitely a discouraged thing to do 😅, but because this function is public, the reason this fails feels like a breach of contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah interesting. that would fail, but I always assume that if you monkeypatch and something fails, that's on you :D the risk and reward of monkeypatching...

But regardless, I agree this one is ok here. If people do want to override log_request, IMO the way to do that is to override the tornado setting log_function instead - and people doing that will not be affected by this change.

I think we can proceed as long as we communicate clearly that there's a "possible breaking change" when this is released.

Is there anything you'd like me to do to make this possible?

Copy link
Member

Choose a reason for hiding this comment

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

if you monkeypatch and something fails, that's on you :D

Agreed 👍

Nope, I think this is good to go.

I'm mostly raised/noted here in the thread so we can cross-link if someone reports a "bug" after release. It's not a bug, but a consequence of a monkeypatch 😃 Documenting this here is enough for future reference.

Thanks @yuvipanda!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yay, ty @Zsailer!

"""log a bit more information about each request than tornado's default

- move static file get success to debug-level (reduces noise)
- get proxied IP instead of proxy IP
- log referer for redirect and failed requests
- log user-agent for failed requests

if record_prometheus_metrics is true, will record a histogram prometheus
metric (http_request_duration_seconds) for each request handler
"""
status = handler.get_status()
request = handler.request
Expand Down Expand Up @@ -97,4 +100,5 @@ def log_request(handler):
headers[header] = request.headers[header]
log_method(json.dumps(headers, indent=2))
log_method(msg.format(**ns))
prometheus_log_method(handler)
if record_prometheus_metrics:
prometheus_log_method(handler)
17 changes: 16 additions & 1 deletion jupyter_server/serverapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import urllib
import warnings
from base64 import encodebytes
from functools import partial
from pathlib import Path

import jupyter_client
Expand Down Expand Up @@ -410,7 +411,9 @@ def init_settings(

settings = {
# basics
"log_function": log_request,
"log_function": partial(
log_request, record_prometheus_metrics=jupyter_app.record_http_request_metrics
),
"base_url": base_url,
"default_url": default_url,
"template_path": template_path,
Expand Down Expand Up @@ -1993,6 +1996,18 @@ def _default_terminals_enabled(self) -> bool:
config=True,
)

record_http_request_metrics = Bool(
True,
help="""
REcord http_request_duration_seconds metric in the metrics endpoint.
yuvipanda marked this conversation as resolved.
Show resolved Hide resolved

Since a histogram is exposed for each request handler, this can create a
*lot* of metrics, creating operational challenges for multitenant deployments.

Set to False to disable recording the http_request_duration_seconds metric.
""",
)

static_immutable_cache = List(
Unicode(),
help="""
Expand Down
Loading