-
Notifications
You must be signed in to change notification settings - Fork 84
feat(server): add SSR React rendering summary metric #991
Conversation
Size Change: 0 B Total Size: 687 kB ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 687 kB ℹ️ View Unchanged
|
https://github.com/americanexpress/one-app/actions/runs/4919317274/jobs/8786780018?pr=991 appears to be unrelated to the changes made? |
const ssrNamespace = createMetricNamespace('ssr'); | ||
|
||
createSummary({ | ||
name: ssrNamespace('react_rendering', 'seconds'), |
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.
are we tracking this in seconds or miliseconds?
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.
seconds is the base unit of time for metrics:
https://prometheus.io/docs/practices/naming/#base-units
from what I can tell they attempt to follow SI https://en.wikipedia.org/wiki/International_System_of_Units
side note: Grafana is used by a lot of different teams, at least one rocket company uses it for their launches (Astra, IIRC)
Yeah: #994 |
|
||
function createSummary(opts) { | ||
const { name } = opts; | ||
if (summaries[name]) { |
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.
this is great for safety but it's hidden, we should instead throw an error as in startSummaryTimer
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.
🤔 mirrors counters.js & gauges.js, maybe a subsequent PR?
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.
ok not a big deal but would be cleaner
|
||
const ssrNamespace = createMetricNamespace('ssr'); | ||
|
||
createSummary({ |
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.
shouldn't the user call this function?
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.
user?
FWIW following the existing pattern, i.e. src/server/metrics/holocron.js
and src/server/utils/pollModuleMap.js
there might be a better way of managing the metrics, but IMO would be best to do that refactor in a different PR rather than block this metric?
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.
by user i mean the entity consuming it (in this case, the dev). If we are following a pattern then ok we could improve the code later
Description
Add a
oneapp_ssr_react_rendering_seconds
summary metric split byrenderMethodName
label.Motivation and Context
Server-Side Rendering (SSR) can be resource intensive. It is useful to operators to know a bit more about the time taken to render the HTML via React.
How Has This Been Tested?
Run locally, comparing manual timers (
process.hrtime.bigint
) to the values visible inhttp://localhost:3005/metrics
.Types of Changes
Checklist:
What is the Impact to Developers Using One App?
DevOps has more telemetry to track server utilization to aid SSR efficiency in modules and possible server scaling.