[Data][Solana] Report Solana batch requests to data pipeline#446
[Data][Solana] Report Solana batch requests to data pipeline#446
Conversation
adshmh
added
solana
| return []*legacyRecord{record} | ||
| } | ||
|
|
||
| // TODO_UPNEXT(@adshmh): Track and report the `method` field of each JSONRPC request in a batch. |
| // 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. |
There was a problem hiding this comment.
Can you add a TODO to ensure different code paths for batch and non batch requests up the stack?
| // Create a separate legacy record for each method | ||
| var legacyRecords []*legacyRecord | ||
| for index := range endpointObservations { | ||
| // Create a copy of the base record |
| 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 |
There was a problem hiding this comment.
Do we have a TODO on when we migrate off of legacy records?
| // Create a copy of the base record | ||
| recordCopy := *record | ||
|
|
||
| // Track the index of the request to ensure correctness of records. |
There was a problem hiding this comment.
This feels wrong. We shouldn't be overriding the ChainMethod.
Do one or more of the following:
- Add a new field
- Log it
- Append it to the existing ChainMethod name
| 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: |
There was a problem hiding this comment.
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"
| 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) |
There was a problem hiding this comment.
I find this confusing.
We have EVMRequestObservation which has this data.
EVMRequestObservation contains an EVMEndpointObervation.
This feels like it would be duplicating the smae metadata.
| // 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. |
There was a problem hiding this comment.
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.
2 participants
Summary
Report Solana batch requests to data pipeline
Primary Changes:
Secondary Changes:
Issue
Each request of a batch JSONRPC request should be reported to the data pipeline.
Type of change
Select one or more from the following:
QoS Checklist
E2E Validation & Tests
make path_upmake test_e2e_evm_shannonObservability
make path_upmake test_request__shannon_relay_util_100Sanity Checklist
assignees,reviewers,labels,project,iterationandmilestonemake docusaurus_startmake test_allTODOs where applicable