Add Stellar WriteReport Beholder telemetry#672
Open
Krish-vemula wants to merge 2 commits into
Open
Conversation
product-security-plaid-production
Bot
requested review from
DylanTinianov,
Farber98,
MStreet3,
Unheilbar,
amit-momin,
dhaidashenko,
fernandezlautaro,
ilija42,
prashantkumar1982,
silaslenihan and
yashnevatia
Emit WriteReport lifecycle, tx hash retrieval, duplicate tx, invalid transmission state, and InvokeOnReport duration events with metrics, processor wiring, and coverage tests aligned with Aptos/EVM patterns.
Krish-vemula
force-pushed
the
feat/stellar-write-beholder-metrics
branch
from
6be63c3 to
8a2f162
Compare
Resolve GetLatestLedger conflict by keeping hash-based consensus from main (#673).
|
ilija42
reviewed
Jul 6, 2026
Comment on lines
+70
to
+76
| case *WriteReportTxHashRetrievalPhase: | ||
| if err := p.Metrics.OnWriteReportTxHashRetrievalPhase(ctx, msg); err != nil { | ||
| return fmt.Errorf("failed to publish WriteReportTxHashRetrievalPhase metrics: %w", err) | ||
| } | ||
| case *WriteReportInvokeOnReportDuration: | ||
| if err := p.Metrics.OnWriteReportInvokeOnReportDuration(ctx, msg); err != nil { | ||
| return fmt.Errorf("failed to publish WriteReportInvokeOnReportDuration metrics: %w", err) |
| return details, nil | ||
| } | ||
|
|
||
| func (r *TxHashRetriever) emitTxHashRetrievalPhase(ctx context.Context, lookupType, result string, phaseStart time.Time, txHash string) { |
Contributor
There was a problem hiding this comment.
lookupType, result, these should be string enums
| } | ||
| } | ||
| r.lggr.Errorw("No successful transmission found", "txCount", len(details), "transactions", details.String()) | ||
| r.emitTxHashRetrievalPhase(ctx, txHashLookupTypeSuccessful, "NotFound", time.Now(), "") |
Contributor
There was a problem hiding this comment.
- Right before this emitTxHashRetrievalPhase is emitted with Found and then here its emitted as NotFound. Just think from the perspective of wanting to debug something and what metric would be useful, this would be really confusing both with naming and chaotic attribute values that are inconsistently populated
- What does time.Now() tell us here?
| basic capmonitoring.MetricsCapBasic | ||
| } | ||
| WriteReportTxHashRetrievalPhase struct { | ||
| count metric.Int64Counter |
Contributor
There was a problem hiding this comment.
What is the value of count here, this is a metric that gets emitted every time, I can see querying it to see how long the retrieval phase lasts on avg, but no need to count something that always happens
| } | ||
| WriteReportTxHashRetrievalPhase struct { | ||
| count metric.Int64Counter | ||
| phaseDuration metric.Int64Histogram |
|
|
||
| func (r *WriteReportTxHashRetrievalPhase) MetricAttributes() []attribute.KeyValue { | ||
| return append([]attribute.KeyValue{ | ||
| attribute.String("phase", r.GetPhase()), |
Contributor
There was a problem hiding this comment.
Phase is always hardocoded to EventPoll, what is the value of this attribute?
| func (r *WriteReportTxHashRetrievalPhase) MetricAttributes() []attribute.KeyValue { | ||
| return append([]attribute.KeyValue{ | ||
| attribute.String("phase", r.GetPhase()), | ||
| attribute.String("result", r.GetResult()), |
Contributor
There was a problem hiding this comment.
This attribute is too broad in my opinion wdyt? We can log it, but I don't think that its a useful metric, look at our dashboard to get a feel for what we usually look for in metrics, this is too specific.
Contributor
|
Submitting first part of review, the tx retrieval metric looks like its copied from Aptos, the diffrence is that Aptos tx lookup is complicated and it makes way more sense to keep track of whats happening, here most of the attributes for this metric would be dead weight with high cardinality while not providing much useful insight into the WriteReport lifecycle |
ilija42
requested changes
Jul 6, 2026
| return details, nil | ||
| } | ||
|
|
||
| func (r *TxHashRetriever) emitTxHashRetrievalPhase(ctx context.Context, lookupType, result string, phaseStart time.Time, txHash string) { |
| func appendUserErrorLogAttributes(reqAttrs []attribute.KeyValue, summary string, isUserError bool, ec *capmonitoring.ExecutionContext) []attribute.KeyValue { | ||
| return append(append(reqAttrs, | ||
| attribute.String("summary", summary), | ||
| attribute.Bool("isUserError", isUserError), |
| func (r *WriteReportInvalidTransmissionState) LogAttributes() []attribute.KeyValue { | ||
| return append(append(writeReportRequestLogAttributes(r.Req), | ||
| attribute.String("summary", r.GetSummary()), | ||
| attribute.String("transmission_state", strconv.FormatUint(uint64(r.GetTransmissionState()), 10)), |
Contributor
There was a problem hiding this comment.
attribute.String is not right for numerical transmission state
| return ec.MetricsAttributes() | ||
| } | ||
|
|
||
| func appendExecutionLogAttributes(reqAttrs []attribute.KeyValue, ec *capmonitoring.ExecutionContext) []attribute.KeyValue { |
| } | ||
| } | ||
|
|
||
| func executionMetricAttributes(ec *capmonitoring.ExecutionContext) []attribute.KeyValue { |
|
|
||
| const ( | ||
| reportProcessedTopicPrefix = "forwarder_ReportProcessed" | ||
| defaultForwarderLookbackLedgers = int64(100) |
Contributor
There was a problem hiding this comment.
This constant is duplicate and is in two differnt places
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WriteReport,tx hash retrieval phases, andInvalidTransmissionStateeventsDepends on
feat/stellar-write-capability- PR (stacked PR merge that first)