feat: track json-rpc metrics - #25159
Conversation
3580550 to
60b2228
Compare
60b2228 to
10007f1
Compare
| ); | ||
| if (apiKeyResolution) { | ||
| adminMiddlewares.unshift(getApiKeyAuthMiddleware(apiKeyResolution.apiKeyHash)); | ||
| adminMiddlewares.splice(1, 0, getApiKeyAuthMiddleware(apiKeyResolution.apiKeyHash)); |
There was a problem hiding this comment.
Is this because we want the api key auth to run after metrics? If so, let's add it in order in the adminMiddlewares above, this splice(1) seems not very robust. I'm thinking something like:
const adminMiddlewares = [
getOtelJsonRpcServerMetricsMiddleware(),
...(apiKeyResolution ? [getApiKeyAuthMiddleware()] : []),
getOtelJsonRpcPropagationMiddleware(),
getVersioningMiddleware(versions, versioningOpts),
];
| ...(service === undefined ? {} : { [ATTR_JSONRPC_SERVICE]: service }), | ||
| [ATTR_JSONRPC_METHOD]: method, |
There was a problem hiding this comment.
Can this be a problem in terms of attribute cardinality if we log every request done, even when the service/method doesn't exist? Eg if I start poking an RPC endpoint with aztec_1, aztec_2, etc, can I screw up prometheus?
| const method = this.handler[methodName as keyof T]; | ||
| assert(typeof method === 'function', `Method ${methodName} is not a function`); | ||
| const args = await parseWithOptionals(jsonParams, getSchemaParameters(this.schema[methodName])); | ||
| const validationTimer = new Timer(); |
There was a problem hiding this comment.
Should we set validationSucceeded to false in these branches? Or do we have something else that collects exceptions and flags the request as failed?
spalladino
left a comment
spalladino
left a comment
There was a problem hiding this comment.
Claude also flagged the following, meat-proxying:
Two rejection reasons are unreachable, and real internal errors never appear in the metric — telemetry-client/src/json_rpc_server_metrics.ts:119-146. Handler-thrown errors (including BadRequestError) are caught inside processRequest and mapped to -32701/-32702, which getRejectionReasons ignores. The -32000 and -32600 + status 500 branches only fire for errors thrown outside processRequest, which nothing currently does. So an operator alerting on rejection_reason=internal_error never trips it even while every handler is throwing. Conversely, an uncaught rejection inside a batch item comes out -32600 / HTTP 200 and gets mislabeled invalid_request. Either drop the dead reasons or classify from the codes the server actually emits.
2 participants
Add metrics to the JSON-RPC server.
Fix A-1677.