feat: add traces for monitored transaction #350
Conversation
|
Go Test coverage is 63.0 %\ ✨ ✨ ✨ |
|
Go Test coverage is 63.0 %\ ✨ ✨ ✨ |
P1sar
requested review from
mpetrun5,
tcar121293,
MakMuftic and
nmlinaric
as code owners
|
Go Test coverage is 63.1 %\ ✨ ✨ ✨ |
|
Go Test coverage is 63.8 %\ ✨ ✨ ✨ |
|
Go Test coverage is 63.8 %\ ✨ ✨ ✨ |
|
|
||
| // CreateSpanAndLoggerFromContext creates span and logger from context with provided name. | ||
| // Logger explicitly extended with dd.trace_id attribute for DataDog logs and traces connections | ||
| func CreateSpanAndLoggerFromContext(ctx context.Context, tracerName, spanName string, kv ...attribute.KeyValue) (context.Context, traceapi.Span, zerolog.Logger) { |
There was a problem hiding this comment.
I would rather use this and pass context into logger when needed rather than to create this functions that realistically do 2 things
There was a problem hiding this comment.
Though I might be fine with it in core, in Sygma I would for sure just use span directly and add context to info logs where needed..
There was a problem hiding this comment.
If you really wanna go this route then something like this would make sense though personally it might be overkill:
type Observability struct {
logger
tracer
}
NewObservability(ctx, tracer, span, ...kv)
func (o Observability) RecordEvent(event...)
func (o Observability) RecordError(error...)
func (o Observability) SetAttributes(kv...)
There was a problem hiding this comment.
- This function actually creates a logger from the existing context, so this API is available to use inside the code if necessary.
- Not sure what you mean here. But if I understand you right logger and span are available when using this utils functions, so when necessary you still can use standard API since this is just no more then an instrument
- I do not see the necessity to do this, we would need to pass this structure everywhere, while we just do not need to store this state. We have context everywhere, which is a pretty default pattern + traces is available globally.
There was a problem hiding this comment.
I don't like how it looks.
And again when something says it does 2 things it probably shouldn't.
There was a problem hiding this comment.
Right, but two different, unrelated ,things. But in our case this instrument do only actions related to observability, logs, and traces.
chains/evm/calls/events/listener.go
Outdated
| if err != nil { | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| return nil, err | ||
| return nil, observability.LogAndRecordErrorWithStatus(&logger, span, err, "failed FetchEventLogs") |
There was a problem hiding this comment.
Most of the retuns are overkill.
It is really not intuitive to log and record every error for every return path when you log it as well when it is propagated.
We didn't log them before and setting span status is overkill as we set span status in the event handler anyway.
I would personally remove the the log and record functions.
If we need both I would prefer to have it explicitly.
There was a problem hiding this comment.
It is not Logging every error, in cases when an error is going to bubble up higher you can just pass nil to the logger, it is described in func readme.
This one really goes up the stack so I will replace logger with nil
There was a problem hiding this comment.
I am more talking about the logic specifically, not necessarily what is logged and traced.
We don't need to trace every error as well as we also have that in the parent span.
There was a problem hiding this comment.
I do not agree, it is pretty useful to mark errored span, since they are not linear and one parent span can hame multiple children, UI nicely displays errored spans with colours which dramatically improves debugging UX
There was a problem hiding this comment.
I agree with you generally, but you have the weigh the options of future maintability and code cleanliness with the information gained.
You see the error anyway where it happened because it is always from the last span which is nicely visualized.
Ideally we would trace every action that the system makes if there is no penalty, but there obviously is.
There was a problem hiding this comment.
Yeah, but it submits traces in batches within a designated time period, so adding more traces is not going to dramatically influence performance.
There was a problem hiding this comment.
I was more talking about the cognitive load needed for someone to understand why do we have a helper function for returning errors.
It is a future nightmare to maintain the code and it is not very clear why do we have it..
Most of my problems with the implementation are that is not very readable and clearly understood why is something being done like that and when.
The code should always be clear and easy to understand to someone who doesn't have the context of why is something being done that way.
There was a problem hiding this comment.
the function itself is pretty clear I do not see any reason that someone would not understand what is it doing.
|
Go Test coverage is 63.8 %\ ✨ ✨ ✨ |
|
Go Test coverage is 63.7 %\ ✨ ✨ ✨ |
|
Go Test coverage is 63.7 %\ ✨ ✨ ✨ |
Added traces to Transactor interface implementation to have more information about the transaction-sending process. What is more important traces were added to the Monitor function that monitors pending transactions and resends them with updated gasPrice if necessary
Description
Related Issue Or Context
Related to: sygmaprotocol/sygma-relayer#205
How Has This Been Tested? Testing details.
Types of changes
Checklist: