Skip to content
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

In pprofextended proto, consider moving Location.type_index to Mapping #561

Closed
aalexand opened this issue May 18, 2024 · 5 comments
Closed

Comments

@aalexand
Copy link

We define it as

  // Type of frame (e.g. kernel, native, python, hotspot, php). Index into string table.
  uint32 type_index = 6;

I can't easily think of a case where the type of locations would be different for the given mapping. This could probably be moved to the mapping message which would also save some profile size since locations tend to come in much larger quantities compared to mappings.

Related: Could this type be an attribute? Both mapping and location have attributes attached.

@aalexand
Copy link
Author

Perhaps one case of having different type of locations per mapping could be if they are language related. E.g. C++ and Rust linked into the same static binary.

@tigrannajaryan
Copy link
Member

@open-telemetry/profiling-maintainers

@christos68k
Copy link
Member

christos68k commented May 21, 2024

Another use case is mixed stacktraces, example: Stacktrace with both native and interpreted (Python) frames. Using a frame type per Location, we'd either keep the same Mapping for both native and interpreted Locations or not have a Mapping for interpreted Locations.

If we lift Location type to Mapping, then we'd need to introduce Mappings for these interpreted frames which seems semantically wrong.

@aalexand
Copy link
Author

Another use case is mixed stacktraces, example: Stacktrace with both native and interpreted (Python) frames. Using a frame type per Location, we'd either keep the same Mapping for both native and interpreted Locations or not have a Mapping for interpreted Locations.

If we lift Location type to Mapping, then we'd need to introduce Mappings for these interpreted frames which seems semantically wrong.

I would probably expect two different "fake" mappings to be used in this case - one for interpreted locations, another for JIT locations. Having separate mappings would allow them to have different names (in the Mapping.filename field) which is useful from the tooling / analysis perspective: tools rendering profiles often expose mapping name in a tooltip on the flame graph and easily seeing from the mapping name whether the location is interpreted or JIT is convenient. pprof also takes the mapping name into account when handling its filters, so using something like -focus=Interpreted would allow to filter the data to just stacks with interpreted frames.

@aalexand
Copy link
Author

With #578 (which is about to be merged) this is moot / not relevant, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants