profiles: Add normalized address field

[brancz]

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)

[christos68k]

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.

[brancz]

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.

[christos68k]

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?

[brancz]

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.

[felixge]

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?

[brancz]

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.

[r1viollet]

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.

[christos68k]

Thanks for the input @r1viollet! To summarize, the crystallized options that seem most appropriate (ignoring backend normalization) are:

1. Store a normalized address boolean in Mapping
2. 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.

[r1viollet]

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.

[brancz]

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).

[aalexand]

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.

[aalexand]

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.

[aalexand]

Please keep comments in 80 columns like in other Otel protos, like this one.

[aalexand]

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.

[r1viollet]

+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.

[aalexand]

I think "normalized" term is heavily overloaded. It would be good to make this more specific.

[aalexand]

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.

[athre0z]

I discussed this internally with the profiling folks at Elastic and we agree that a dedicated field isn't really necessary.

I suspect that what lead @brancz to suggest this field is that in the donated OTel profiling agent we incorrectly populated the Mapping record fields memory_start, memory_limit, and file_offset. We send out all sample addresses rebased into the corresponding ELF's VA space (just like @aalexand describes is the case for pprof), but left the previously mentioned fields either uninitialized (0) or at bogus values (in case of file_offset).

Instead of defining a complex normalization schema here, I started working on a PR that correctly initializes the fields, just like pprof tooling does. With these fields initialized properly, any consumer should have enough information to convert the sample addresses into whatever address space they wish using the schema described by @aalexand here.

[aalexand]

I discussed this internally with the profiling folks at Elastic and we agree that a dedicated field isn't really necessary.

I suspect that what lead @brancz to suggesting this field is that in the donated OTel profiling agent we incorrectly populated the Mapping record fields memory_start, memory_limit, and file_offset. We send out all sample addresses rebased into the corresponding ELF's VA space (just like @aalexand describes is the case for pprof), but left the previously mentioned fields either uninitialized (0) or at bogus values (file_offset).

Instead of defining a complex normalization schema here, I started working on a PR that correctly initializes the fields, just like pprof tooling does. With these fields initialized properly, any consumer should have enough information to convert the sample addresses into whatever address space they wish using the schema described by @aalexand here.

Great, good to hear! We can probably improve the docs to describe better recommendations to collector authors / owners. There are only two hard problems about building a profiler after all: stack unwinding and dealing with ELF offsets.

[brancz]

I'm perfectly happy with fully populating locations since that's what we eventually also opted to doing in Parca Agent (specifically to be fully pprof compatible, we originally also stabilized addresses). However, I think having the stabilized address optionally could be interesting, as the whole reason was to be able to combine/use it with stack IDs that would be stable across hosts even in the presence of ASLR so not the whole stack would need to be sent across the wire.

[aalexand]

I'm perfectly happy with fully populating locations since that's what we eventually also opted to doing in Parca Agent (specifically to be fully pprof compatible, we originally also stabilized addresses). However, I think having the stabilized address optionally could be interesting, as the whole reason was to be able to combine/use it with stack IDs that would be stable across hosts even in the presence of ASLR so not the whole stack would need to be sent across the wire.

Even if a collector decides to put in elf-header-normalized addresses into profiles, I don't think the proposed field is needed. The profile would just record adjusted sample and mapping addresses. Consumers shouldn't care.

[brancz]

Perfectly happy with that but how to consumers know not to double apply then?

[aalexand]

Perfectly happy with that but how to consumers know not to double apply then?

Attempting to apply it again will detect that the mapping load address already matches the load address in the ELF segment header, so the adjustment will be zero and there is nothing to apply.

[athre0z]

I'm perfectly happy with fully populating locations since that's what we eventually also opted to doing in Parca Agent (specifically to be fully pprof compatible, we originally also stabilized addresses). However, I think having the stabilized address optionally could be interesting, as the whole reason was to be able to combine/use it with stack IDs that would be stable across hosts even in the presence of ASLR so not the whole stack would need to be sent across the wire.

Hmm, I think there might be some confusion with what ELF address space means here. An address in ELF address space is stabilized and generalizes across hosts. It's the address that is used in the executable itself before relocations are applied, and the addresses that all common executable analysis tooling (binutils, llvm-dwarfdump, Ghidra, IDA, ...) will display. It's stable across hosts.

What you seem to be referring to here would be the mapped / process address which will vary depending on ASLR bias. I'm not proposing to send samples in this address space in the OTel profiler and I also wouldn't recommend doing that to any other implementation. You'd essentially be leaking the address layout in your production cluster to whomever has access to your profiling data; it could be considered an exploitable information leak (circumvents ASLR).

However, technically with the schema where we send the memory_start, memory_limit, and file_offset fields along with the address, in terms of the ability to normalize across hosts it really doesn't matter what address space an implementation chooses to send samples with, because you have all the required information to transform it into another one on the collector side.

Perhaps we should simply change documentation and strongly encourage implementations to transform addresses into some form of stable address space?

[aalexand]

Perhaps we should simply change documentation and strongly encourage implementations to transform addresses into some form of stable address space?

This SGTM. The per-file/mapping ELF address space is probably the most practical choice as you said.

[brancz]

Got it, that works. I’d say that should be a separate PR then.