-
Notifications
You must be signed in to change notification settings - Fork 7
[Data][Solana] Report Solana batch requests to data pipeline #446
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,9 +35,7 @@ func setLegacyFieldsFromQoSObservations( | |
|
|
||
| // Use Solana observations to update the legacy record's fields. | ||
| if solanaObservations := observations.GetSolana(); solanaObservations != nil { | ||
| populatedRecord := setLegacyFieldsFromQoSSolanaObservations(logger, baseLegacyRecord, solanaObservations) | ||
| // Solana does not support batch requests so expect a single record. | ||
| return []*legacyRecord{populatedRecord} | ||
| return setLegacyFieldsFromQoSSolanaObservations(logger, baseLegacyRecord, solanaObservations) | ||
| } | ||
|
|
||
| // Use Cosmos SDK observations to update the legacy record's fields. | ||
|
|
@@ -146,13 +144,13 @@ func populateEVMErrorFields(legacyRecord *legacyRecord, evmInterpreter *qosobser | |
| // Returns: the populated legacy record | ||
| func setLegacyFieldsFromQoSSolanaObservations( | ||
| logger polylog.Logger, | ||
| legacyRecord *legacyRecord, | ||
| record *legacyRecord, | ||
| observations *qosobservation.SolanaRequestObservations, | ||
| ) *legacyRecord { | ||
| ) []*legacyRecord { | ||
| logger = logger.With("method", "setLegacyFieldsFromQoSSolanaObservations") | ||
|
|
||
| // In bytes: the length of the request: float64 type is for compatibility with the legacy data pipeline. | ||
| legacyRecord.RequestDataSize = float64(observations.RequestPayloadLength) | ||
| record.RequestDataSize = float64(observations.RequestPayloadLength) | ||
|
|
||
| // Initialize the Solana observations interpreter. | ||
| // Used to extract required fields from the observations. | ||
|
|
@@ -162,24 +160,49 @@ func setLegacyFieldsFromQoSSolanaObservations( | |
| } | ||
|
|
||
| // Extract the JSONRPC request's method. | ||
| legacyRecord.ChainMethod = solanaInterpreter.GetRequestMethod() | ||
| record.ChainMethod = solanaInterpreter.GetRequestMethod() | ||
|
|
||
| // ErrorType is already set at gateway or protocol level. | ||
| // Skip updating the error fields to preserve the original error. | ||
| if legacyRecord.ErrorType != "" { | ||
| return legacyRecord | ||
| if record.ErrorType != "" { | ||
| return []*legacyRecord{record} | ||
| } | ||
|
|
||
| // TODO_UPNEXT(@adshmh): Track and report the `method` field of each JSONRPC request in a batch. | ||
| // - This requires updating the gateway.QoSRequestContext interface to guarantee a 1:1 map between requests of a batch and responses. | ||
| // | ||
| // TODO_TECHDEBT(@adshmh): Track each request of a batch JSONRPC request separately in proto messages. | ||
| // TODO_TECHDEBT(@adshmh): Include a num_requests fields for batch JSONRPC requests once data pipeline is refactored. | ||
| endpointObservations := observations.GetEndpointObservations() | ||
| // 0 or 1 endpoint observations: not a batch JSONRPC request. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a TODO to ensure different code paths for batch and non batch requests up the stack? |
||
| if len(endpointObservations) <= 1 { | ||
| return []*legacyRecord{record} | ||
| } | ||
|
|
||
| errType := solanaInterpreter.GetRequestErrorType() | ||
| legacyRecord.ErrorType = errType | ||
| legacyRecord.ErrorMessage = errType | ||
| // TODO_UPNEXT(@adshmh): Track and report errors on each request of a JSONRPC batch request. | ||
| // | ||
| // Create a separate legacy record for each method | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a TODO on when we migrate off of legacy records? |
||
| var legacyRecords []*legacyRecord | ||
| for index := range endpointObservations { | ||
| // Create a copy of the base record | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #PUC why |
||
| recordCopy := *record | ||
|
|
||
| // Track the index of the request to ensure correctness of records. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels wrong. We shouldn't be overriding the ChainMethod. Do one or more of the following:
|
||
| recordCopy.ChainMethod = fmt.Sprintf("batch_request_index:%d", index) | ||
|
|
||
| legacyRecords = append(legacyRecords, &recordCopy) | ||
| } | ||
|
|
||
| return legacyRecord | ||
| return legacyRecords | ||
| } | ||
|
|
||
| // qosCosmosErrorTypeStr defines the prefix for Cosmos QoS error types in legacy records | ||
| const qosCosmosErrorTypeStr = "QOS_COSMOS" | ||
|
|
||
| // TODO_TECHDEBT(@adshmh): Refactor the data reporting logic: | ||
| // - QoS logic should simply return a list of legacy records (to eliminate the need for copying the original record) | ||
| // - Protocol and Gateway logic sets the fields related to their perspective on the QoS returned set of records. | ||
| // | ||
| // setLegacyFieldsFromQoSCosmosObservations populates legacy records with Cosmos SDK-specific QoS data. | ||
| // It captures: | ||
| // - Request payload size (aggregated across all request profiles) | ||
|
|
@@ -217,6 +240,8 @@ func setLegacyFieldsFromQoSCosmosObservations( | |
| return []*legacyRecord{baseLegacyRecord} | ||
| } | ||
|
|
||
| // TODO_TECHDEBT(@adshmh): Refactor to loop over the endpoint observations instead. | ||
| // | ||
| // Create a separate legacy record for each method | ||
| // This enables the data pipeline to track metrics per individual method | ||
| // Similar to EVM batch request handling | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,9 @@ import "path/qos/cosmos_response.proto"; | |
| import "path/qos/request_origin.proto"; | ||
| import "path/qos/request_error.proto"; | ||
|
|
||
| // TODO_TECHDEBT(@adshmh): Reorganize the messages to be consistent with both single and batch JSONRPC requests: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully understand this TODO. IMO it should say something similar to "Create an independent Request Observation for each request in a batch" |
||
| // - Directly associate each request of a batch with the corresponding endpoint observation(s). | ||
| // | ||
| // CosmosRequestObservations captures all observations made while serving a single Cosmos blockchain service request. | ||
| message CosmosRequestObservations { | ||
| // Next free index: 10 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,6 +164,9 @@ message EVMRequestUnmarshalingFailure { | |
| optional string error_details = 3; | ||
| } | ||
|
|
||
| // TODO_TECHDEBT(@adshmh): Enhance the endpoint observation to include the corresponding request's details (e.g. method field of JSONRPC) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this confusing. We have
This feels like it would be duplicating the smae metadata. |
||
| // This will enable tracking each request of a batch of JSONRPC request alongside the endpoint's response. | ||
| // | ||
| // EVMEndpointObservation stores a single observation from an endpoint servicing the protocol response. | ||
| // Example: A Pocket node on Shannon backed by an Ethereum data node servicing an `eth_getBlockNumber` request. | ||
| message EVMEndpointObservation { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ type batchJSONRPCRequestContext struct { | |
| // - QoS: requests built by the QoS service to get additional data points on endpoints. | ||
| requestOrigin qosobservations.RequestOrigin | ||
|
|
||
| // endpointResponses is the set of responses received from one or | ||
| // endpointJSONRPCResponses is the set of responses received from one or | ||
| // more endpoints as part of handling this service request. | ||
| endpointJSONRPCResponses []endpointJSONRPCResponse | ||
| } | ||
|
|
@@ -130,6 +130,11 @@ func (brc batchJSONRPCRequestContext) GetHTTPResponse() pathhttp.HTTPResponse { | |
| } | ||
| } | ||
|
|
||
| // TODO_IMPROVE(@adshmh): Track the method field of each request in a JSONRPC batch request: | ||
| // - Update proto/path/qos/solana.proto to include request details in each endpoint observation. | ||
| // - Map each request in a batch to its corresponding response: needs gateway.QoSRequestContext interface update to handle slice of response. | ||
| // - Update the endpoint observation building code below to include details of the corresponding request. | ||
| // | ||
| // GetObservations returns all the observations contained in the request context. | ||
| // Implements the gateway.RequestQoSContext interface. | ||
| func (rc batchJSONRPCRequestContext) GetObservations() qosobservations.Observations { | ||
|
|
@@ -154,8 +159,32 @@ func (rc batchJSONRPCRequestContext) GetObservations() qosobservations.Observati | |
| } | ||
| } | ||
|
|
||
| // TODO_UPNEXT(@adshmh): Report batch JSONRPC requests endpoint observations via metrics. | ||
| // | ||
| // Add one endpoint observation per request in the JSONRPC batch request. | ||
| endpointObservations := make([]*qosobservations.SolanaEndpointObservation, len(rc.endpointJSONRPCResponses)) | ||
| for index, endpointResp := range rc.endpointJSONRPCResponses { | ||
| // TODO_TECHDEBT(@adshmh): Support method-specific JSONRPC responses on batch requests. | ||
| // This requires mapping each endpoint response to its corresponding request in the batch. | ||
| // | ||
| endpointObs := &qosobservations.SolanaEndpointObservation{ | ||
| // TODO_DOCUMENT(@adshmh): Add a reference for the choice of HTTP status code on batch requests. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully understand why we're calling everything 200 right now. Please explain. I feel like we already have a mapper somewhere we can use. |
||
| // | ||
| // HTTP status code 200 for batch requests. | ||
| HttpStatusCode: int32(http.StatusOK), | ||
| // Track response as an unrecognized response, since QoS does not currently use batch requests to evaluate endpoints. | ||
| ResponseObservation: &qosobservations.SolanaEndpointObservation_UnrecognizedResponse{ | ||
| UnrecognizedResponse: &qosobservations.SolanaUnrecognizedResponse{ | ||
| // Track details of the JSONRPC response: e.g. ID and a preview of result. | ||
| JsonrpcResponse: endpointResp.GetObservation(), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Store in the list of endpoint observations. | ||
| endpointObservations[index] = endpointObs | ||
| } | ||
|
|
||
| observations.EndpointObservations = endpointObservations | ||
|
|
||
| return qosobservations.Observations{ | ||
| ServiceObservations: &qosobservations.Observations_Solana{ | ||
| Solana: observations, | ||
|
|
||
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.
Great TODOs.