-
Notifications
You must be signed in to change notification settings - Fork 9
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
Added support for propagating server timing metrics to client #386
Conversation
…ent this in separate PR instead
|
||
def set_metric(self, metric_name: str, duration_ms: int) -> None: | ||
int_duration_ms = int(duration_ms) | ||
self._metrics_dict[metric_name] = int(int_duration_ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should at least already be sure int_duration_ms
is of type int
? 😄
def get_as_string(self) -> str: | ||
return ", ".join([f"{key}={value}ms" for key, value in self._metrics_dict.items()]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think __str__
would be more Pythonic:
def get_as_string(self) -> str: | |
return ", ".join([f"{key}={value}ms" for key, value in self._metrics_dict.items()]) | |
def __str__(self) -> str: | |
return ", ".join([f"{key}={value}ms" for key, value in self._metrics_dict.items()]) |
Then it should be called automatically by Python when needing to convert the object instance to a string in various contexts. If we include ms-output as well in the generated string we can simplify lines like
LOGGER.debug(f"Loaded property surface in: {perf_metrics.get_elapsed_ms()}ms ({perf_metrics.get_as_string()})")
which we probably will get many of since they are defined in each endpoint, to something simpler like
LOGGER.debug(f"Loaded property surface in: {perf_metrics}")
which would also ensure we print out the performance metric instance in a consistent way across the app endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went for a hybrid, to_string(self, include_total_elapsed: bool = True)
, that by default includes the total elapsed time and can be used directly in log statements.
Added two utility classes,
AddProcessTimeToServerTimingMiddleware
andPerfMetrics
and that can be used to gather and propagate server timing metrics to the client.Both these clases utilize the Server-Timing response header to communicate the chosen metrics.
The
PerfMetrics
class can also be used to gather metrics for logging in the backend logs.Added example usage of
PerfMetrics
inbackend/primary/routers/surface/router.py