-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First design for OpenTelemtry #82
Conversation
4c72327
to
0285d73
Compare
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.
Looks good, left comment about possible improvements in the future
@@ -176,19 +177,37 @@ func FheLibRun(environment EVMEnvironment, caller common.Address, addr common.Ad | |||
logger.Error("fheLib precompile error", "err", err, "input", hex.EncodeToString(input)) | |||
return nil, err | |||
} | |||
otelCtx := environment.OtelContext() | |||
tracer := otel.Tracer("fhevm") | |||
// first 4 bytes are for the function signature | |||
signature := binary.BigEndian.Uint32(input[0:4]) | |||
switch signature { |
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'm the author of the initial switch statement (looked good idea at the time for me) and maybe in the future we can replace this with a map of small abstraction, like structs of all the fhevm functions which contain, say
type fhevmPrecompileMethod {
name string
implementation <function signature which implements current switch block>
}
And have a generated map of these for first 4 bytes of method signature
map[[]byte]fhevmPrecompileMethod
Then we could just do one lookup to the map, and do one trace using name of the method not to repeat ourselves.
Also, in future bwCompatBytes logic could be just moved inside implementation method, say fheAddRun
.
This was needed before when we supported separate precompile smart contracts instead of one, we no longer need that now
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.
Yes, this seems a nice refactor, I will do that just after we merge this (probably today)
EVMEnvironment
with a method to get the EVM context using in Otel (returningnil
means it's disabled)This is an initial design that doesn't have to make a lot of changes to the codebase, we can extend it later with more events that are needed for observability.
Note: This is a breaking change as it introduce a new method to the
EVMEnvironment
interface. We should update the docs to take this into account and explain how someone could use Otel withfhevm-go
.