profiles: Add normalized address field #553
Conversation
| // execution-context agnostic. This is important, because a symbolizer needs | ||
| // this information to correctly process an address or not prior to | ||
| // symbolization. | ||
| bool normalized_addresses = 19; |
There was a problem hiding this comment.
Should this be a profile, sample or location attribute?
A profile may include samples of different types, and I'm unsure if normalized addresses on this level place undue restrictions on the implementor. Also, we should probably be more specific and describe the normalization scheme in Location.address.
There was a problem hiding this comment.
In theory, it could be a per-location thing, but the only examples we've seen in the wild are either everything in a profile or nothing since it's more of a "producer concern" than the location itself.
I agree describing the actual normalization scheme here makes sense. I'll add a link to the pprof base address calculation.
There was a problem hiding this comment.
Making it a Location attribute is probably excessive/too-fine-grained (does it make sense to allow semantic changes for frame addresses within the scope of a single sample?), what do you think about making it a Sample attribute instead?
This question has more to do with what we expect going forward, on the part of SDKs/implementors, since not a lot of folks are using this signal yet. If we make it a Profile-level attribute, then we'd be forcing everyone to split data with different semantics for frame addresses across Profile boundaries, thus missing out on e.g. unified string table.
@felixge @petethepig what do you think?
There was a problem hiding this comment.
Yeah, I see where you're coming from. Per sample would work as well, but of course adds additional bytes to each sample. I don't feel too strongly either way, I was just going to go with this since we've only ever seen it one way or the other for the whole profile, and I can't come up with why a counter-example would make sense.
There was a problem hiding this comment.
I'm not very familiar with address normalization, so apologies if this makes no sense, but wouldn't the Mapping be the right level for storing this?
There was a problem hiding this comment.
Agreed with @christos68k. It describes a property of the Location's address.
I'd be ok with it (it being whether the location's address is normalized) being in the location, or if we think that's excessive in terms of space-use in the wire protocol for the sample or whole profile.
There was a problem hiding this comment.
Thanks for the proposal, I agree that normalizing addresses on the fly can make things simpler. It is not always easy to retrieve matching binaries. Having consistent addresses helps with investigations and offline symbolization.
Having a boolean at the level of the profile is slightly strange, as we will have inconsistent behaviors depending on binaries.
Carrying this information at the level of the mapping is the most efficient way to store this information. We can either:
-
Consider storing a boolean
The boolean would indicate if a normalization process was successfully done on the addresses stored in the locations (for the given mapping). Though I agree this is strange considering @christos68k ‘s comment. -
Store the result of the GetBase for this mapping to enable the computation of the normalized address.
This still requires extra steps from the user to recompute the result of the normalization, though this ensures we have all the information to go from one address to another.
Putting the information at the level of the location seems slightly wasteful. I do not think it is possible to have cases where the normalization is different for locations within the same mapping. Although it does make things easier when looking at raw data (as you no longer need to use the adjustment from the mapping). In standard cases, we have ~1000s of locations, so this would add extra tens of kilobytes. If we start having millions of locations (aggregating over longer periods?unusual workloads?), this could become a reasonable optimization to consider.
There was a problem hiding this comment.
Thanks for the input @r1viollet! To summarize, the crystallized options that seem most appropriate (ignoring backend normalization) are:
- Store a normalized address boolean in
Mapping - Store a normalized address boolean in
Sample
For 1., normalization state would apply to all Locations in Mapping.
For 2., normalization state would apply to all Mappings and Locations in Sample.
For both options we should document that if the boolean is true, indicating normalized addresses, Mapping.memory_start / Mapping.memory_limit could be missing or set to zero. It's not always convenient or even needed to collect these and this is one way to make them optional. That should address possible confusion regarding these values.
If we don't expect to see (or need) Mappings with different normalization states in the same Sample, then 2. is slightly more efficient. I'm looking at this from the perspective of a continuous profiler that is in full control of extracting mappings and performing normalization, for which case it'd never need to change normalization state for Mappings in the same Sample (or even in the same Profile) but of course that's not the only use-case we want to cover.
There was a problem hiding this comment.
Trying to think of use cases where normalization can go wrong in profiling, I think temporary extracted libraries could fall into this category:
You have native libraries that are created as temporary files, loaded, then deleted. You will find them with names like: ibnetty_tcnative_linux_x86_12345.so in java, in a tmp folder.
If you profile after the load, you would not find the elf file to perform the normalization (it would be flagged as deleted). Though offline, with the proper logic, you could symbolize using mappings and process level addresses. This could lead to a case where you would have inconsistent normalization states depending on mappings.
There was a problem hiding this comment.
It seems like the most alignment is on having it in Mapping, which I'm fine with, as at least it removes it to be repeated in every Location (although like I said above it actually more describes the Location's address but I don't feel too strongly about that).
Store the result of the GetBase for this mapping to enable the computation of the normalized address.
Part of the whole point of not doing this is to ensure that the address is identical across hosts, other binaries that dynamically link a library, etc. as that opens the door to deduplication (both in the protocol and server side).
brancz
force-pushed
the
address-normalization
branch
from
e3e7349
to
39b401c
Compare
| // symbolization. Normalization means subtracting the base from the address. | ||
| // The base is calculated depending on the executable type: | ||
| // | ||
| // * ET_EXEC (Executable files): For executable files (ET_EXEC), typically no adjustment is necessary as they are loaded at fixed addresses, usually starting at address 0. Therefore, the base is often 0, meaning the virtual address directly corresponds to the symbol table address. |
There was a problem hiding this comment.
I think one note that should be added here is that many executables these days enable ASLR meaning compile as PIE. Their type is ET_DYN in this case. So not every executable in the traditional meaning of this term (the main binary of a process) is ET_EXEC.
| // symbolization. Normalization means subtracting the base from the address. | ||
| // The base is calculated depending on the executable type: | ||
| // | ||
| // * ET_EXEC (Executable files): For executable files (ET_EXEC), typically no adjustment is necessary as they are loaded at fixed addresses, usually starting at address 0. Therefore, the base is often 0, meaning the virtual address directly corresponds to the symbol table address. |
There was a problem hiding this comment.
Strictly speaking, the executable segment is most often loaded from address 0x1000. I think more accurate description would be that the code segment load address in memory is equal to VirtAddr from the PE segment header, so no adjustment is necessary.
| // The base is calculated depending on the executable type: | ||
| // | ||
| // * ET_EXEC (Executable files): For executable files (ET_EXEC), typically no adjustment is necessary as they are loaded at fixed addresses, usually starting at address 0. Therefore, the base is often 0, meaning the virtual address directly corresponds to the symbol table address. | ||
| // * ET_DYN (Dynamic libraries): For dynamic libraries or Position Independent Executables (PIEs), which are marked as ET_DYN, the base address is the virtual address where the mapping of the library or executable starts. This base needs to be calculated to map a runtime virtual address to a symbol address. This is achieved by considering the start of the virtual address range and the offset within the executable file. The calculation adjusts runtime addresses to file offsets, accounting for any discrepancies caused by the location and offset of the executable segment as detailed in the program header. |
There was a problem hiding this comment.
Please keep comments in 80 columns like in other Otel protos, like this one.
| // The base is calculated depending on the executable type: | ||
| // | ||
| // * ET_EXEC (Executable files): For executable files (ET_EXEC), typically no adjustment is necessary as they are loaded at fixed addresses, usually starting at address 0. Therefore, the base is often 0, meaning the virtual address directly corresponds to the symbol table address. | ||
| // * ET_DYN (Dynamic libraries): For dynamic libraries or Position Independent Executables (PIEs), which are marked as ET_DYN, the base address is the virtual address where the mapping of the library or executable starts. This base needs to be calculated to map a runtime virtual address to a symbol address. This is achieved by considering the start of the virtual address range and the offset within the executable file. The calculation adjusts runtime addresses to file offsets, accounting for any discrepancies caused by the location and offset of the executable segment as detailed in the program header. |
There was a problem hiding this comment.
I'm not sure I agree with "The calculation adjusts runtime addresses to file offsets", in particular the usage of "file offsets" term. In pprof, for example, addresses are adjusted to be virtual addresses as if the binary is loaded at the default address described in the PE header. This is basically the address that can be passed to llvm-symbolize or addr2line for symbolization. It may match the file offset if Offset == VirtAddr in the segment PE header, but it doesn't have to, depending on the layout of the binary and linker script.
There was a problem hiding this comment.
+1. It would be nice to have clear separate address space naming. For instance, we defined: file, elf and process space addresses to make this clearer. I am not sure what terminology would work best.
| // * ET_REL (Relocatable files): For relocatable files (ET_REL), the base is typically the starting virtual address of where the file is loaded because these types of files are not bound to a specific address and require relocation processing. | ||
| // | ||
| // An example of calculating the base address can be found in pprof: https://github.com/google/pprof/blob/e4905b036c4ea7eb8e42342ed91e4a6dd5fcadfb/internal/elfexec/elfexec.go#L212-L283 | ||
| bool normalized_addresses = 19; |
There was a problem hiding this comment.
I think "normalized" term is heavily overloaded. It would be good to make this more specific.
| @@ -82,6 +82,18 @@ message Profile { | |||
| repeated Location location = 4; | |||
| // Array of locations referenced by samples. | |||
| repeated int64 location_indices = 15; | |||
| // Whether addresses within locations have been normalized to be | |||
There was a problem hiding this comment.
I'm not convinced from this description that this new field is necessary, to be honest. But I wasn't in the Profiling WG meeting where this was discussed so I probably miss some nuances. It would be good to mention in more detail what functionality or knowledge is missed if this field is not added.
In general, translating a runtime address for a profile into objdump-compatible address goes as profile_address - rtMappingVaddr + peSegmentVaddr + rtMappingFileOff - peSegmentFileOff. If a particular profile translation step wants to actually rewrite addresses in the profile into their objdump-compatible versions, then I think the same step should be able to set Mapping.memory_start, Mapping.memory_limit and Mapping.file_offset fields into their values from the PE header so that the above formula in downstream processing yields zero offset.
As discussed in the last otel profiling call. The comment should have all the context for why this is needed, if not then let me know and I'll add more detail.
@felixge @florianl @petethepig @tigrannajaryan @jack-berg @simonswine
(feel free to tag anyone else)