-
Notifications
You must be signed in to change notification settings - Fork 56
feat: add traces for monitored transaction #350
base: tracing
Are you sure you want to change the base?
Conversation
Go Test coverage is 63.0 %\ ✨ ✨ ✨ |
Go Test coverage is 63.0 %\ ✨ ✨ ✨ |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: