-
Notifications
You must be signed in to change notification settings - Fork 246
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
interpreters, reporter: refactor towards statelessness #171
base: main
Are you sure you want to change the base?
Conversation
4b62fd0
to
4823881
Compare
Due to legacy reasons, each interpreter kept their own state of which dynamic metadata should be sent to the reporter. Several of these caches would never expire, causing caching issues in the otlp reporter module. This removes the caching state from all interpreters and pushes it to the reporter module. A new reporter API call FrameNeeded is added to query if a specific Frame is in the cache or not. Not all interpreter modules use the call as all the information might be available with little overhead. FrameMetadata is also updated to use the FrameID type for symmetry. Improved are: - reduced memory overhead as per-interpreter caches are removed - reporter module can now control which frames need resolving - fixes otlp to get the frames re-symbolized if its internal lru already forgot about the earlier symbolization information ref open-telemetry#121
4823881
to
f87d97b
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.
just a minor nit 👍
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.
Thanks for taking care of this!
Reminder (also to myself):
We need to adapt reporting executable metadata and kernel frames in a similar way (moving caches into the reporter).
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.
Some nits...
interpreter/dotnet/instance.go
Outdated
@@ -764,19 +756,16 @@ func (i *dotnetInstance) Symbolize(symbolReporter reporter.SymbolReporter, | |||
if err != nil { | |||
return err | |||
} | |||
fileID := module.fileID | |||
// The Line ID is the Relative Virtual Address (RVA) within into the PE file |
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 Line ID is the Relative Virtual Address (RVA) within into the PE file | |
// The Line ID is the Relative Virtual Address (RVA) within the PE file |
h := fnv.New128a() | ||
_, _ = h.Write([]byte(name)) | ||
nameHash := h.Sum(nil) | ||
lineID = libpf.AddressOrLineno(npsr.Uint64(nameHash, 0)) |
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.
Does it make sense to have an extra function for this?
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 same pattern is written out in several interpreter modules. It might make sense to make a helper for this in the interpreter
or libpf
module. But I'd make it a separate / follow up PR instead since its logically a separate thing.
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.
LGTM thanks!
Addressed feedback: comments updated and the |
Due to legacy reasons, each interpreter kept their own state of which dynamic metadata should be sent to the reporter. Several of these caches would never expire, causing caching issues in the otlp reporter module.
This removes the caching state from all interpreters and pushes it to the reporter module. A new reporter API call FrameNeeded is added to query if a specific Frame is in the cache or not. Not all interpreter modules use the call as all the information might be available with little overhead. FrameMetadata is also updated to use the FrameID type for symmetry.
Improved are:
ref #121