Emit beholder message for Atlas userOp#447
Conversation
|
2819694 to
3a30336
Compare
8de2701 to
2a3a953
Compare
| req.Header.Add("Content-Type", "application/json") | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| responseReceivedAt := time.Now().UnixMicro() |
There was a problem hiding this comment.
It's used here, I was just being consistent.
There was a problem hiding this comment.
Good point, then I guess there's a reason for it, though I can't remember why we didn't just use UnixMilli()
| a.metrics.RecordBidsReceived(ctx, len(response.Result.SOS)) | ||
| userOpHash := common.Hash{} | ||
| if response.Result.DO != nil { | ||
| userOpHash = response.Result.DO.UserOpHash // |
| UseProtoNames: true, | ||
| EmitUnpopulated: true, | ||
| }.Format(msg) | ||
| m.lggr.Infow("[Beholder.emit]", "message", mStr, "attributes", attrKVs) |
There was a problem hiding this comment.
Maybe this should be debug, it will pollute the logs a lot
| m.lggr.Debugw("Successfully emitted Atlas error event to Beholder", "message", mStr, "attributes", attrKVs) | ||
| } | ||
|
|
||
| func (m *MetaMetrics) emitAtlasUserOp(ctx context.Context, userOpHash common.Hash, tx *types.Transaction, requestSentAt, responseReceivedAt int64) { |
There was a problem hiding this comment.
We're not returning all error conditions, maybe we should return them and in the caller log + ignore?
There was a problem hiding this comment.
Which error conditions are you thinking of? This is in-line with emitAtlasError, unless you're talking about the caller.
There was a problem hiding this comment.
Right yeah, it's indeed in line with emitAtlasError. Ok then let's keep it as-is
There was a problem hiding this comment.
Pull request overview
This PR adds Beholder event emission for Atlas user operation (userOp) data from the dualbroadcast MetaClient flow, enabling downstream observability of userOp hash + timing + lifecycle metadata.
Changes:
- Add
MetaMetrics.emitAtlasUserOpto emitpb.FastLaneAtlasUserOpto Beholder (including lifecycle ID and request/response timestamps). - Emit the userOp event from
MetaClient.SendRequestfor both success and JSON-RPC error responses (when auserOpHashis present). - Add a unit test validating the emitted protobuf payload for the userOp event.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/txm/clientwrappers/dualbroadcast/meta_metrics.go | Implements Beholder emission for FastLaneAtlasUserOp and adjusts Beholder emit logging for errors. |
| pkg/txm/clientwrappers/dualbroadcast/meta_client.go | Captures request/response timestamps and triggers userOp emission on both error and success responses. |
| pkg/txm/clientwrappers/dualbroadcast/meta_metrics_test.go | Adds a unit test for emitAtlasUserOp validating emitted protobuf fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| meta, err := tx.GetMeta() | ||
| if err != nil { | ||
| m.lggr.Errorw("Failed to get meta for tx. UserOpHash to emit was: "+userOpHash.Hex(), "txId", tx.ID, "err", err) |
There was a problem hiding this comment.
In the GetMeta() error path, the log message is built via string concatenation to include the userOpHash. For consistency with other structured logs (and to keep the message stable for log parsing), pass userOpHash as its own key/value field (e.g., "userOpHash", userOpHash.Hex()) instead of embedding it into the message string.
| m.lggr.Errorw("Failed to get meta for tx. UserOpHash to emit was: "+userOpHash.Hex(), "txId", tx.ID, "err", err) | |
| m.lggr.Errorw("Failed to get meta for tx", "txId", tx.ID, "userOpHash", userOpHash.Hex(), "err", err) |
| }) | ||
| } | ||
|
|
||
| func TestMetaClient_emitAtlasUserOp(t *testing.T) { |
There was a problem hiding this comment.
This test function name says "MetaClient" but it’s validating MetaMetrics.emitAtlasUserOp. Renaming it (e.g., to TestMetaMetrics_emitAtlasUserOp) would make failures easier to interpret and keep test naming accurate.
| func TestMetaClient_emitAtlasUserOp(t *testing.T) { | |
| func TestMetaMetrics_emitAtlasUserOp(t *testing.T) { |
| func (m *MetaMetrics) emitAtlasUserOp(ctx context.Context, userOpHash common.Hash, tx *types.Transaction, requestSentAt, responseReceivedAt int64) { | ||
| var nonce string | ||
| if tx.Nonce != nil { | ||
| nonce = strconv.FormatUint(*tx.Nonce, 10) | ||
| } | ||
|
|
||
| meta, err := tx.GetMeta() | ||
| if err != nil { | ||
| m.lggr.Errorw("Failed to get meta for tx. UserOpHash to emit was: "+userOpHash.Hex(), "txId", tx.ID, "err", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
emitAtlasUserOp introduces multiple branches (nil nonce, nil/invalid meta, and emitter.Emit error handling) but the new tests only cover the fully-populated happy path. Adding coverage for the other branches (similar to the existing emitAtlasError tests) would help prevent regressions in these error-handling paths.
Depends on (need to bump dep ref after merge):
This PR enables sending userOp data.