[pdata/pprofile] Add reference based attributes support

[florianl]

Description

Draft implementation for open-telemetry/opentelemetry-proto#733 that shows the transparent conversion between reference based attributes and pdata API.

With open-telemetry/opentelemetry-collector-contrib#46331 there is the respective change that fixes the failing tests in connector/countconnector.

FYI: @open-telemetry/profiling-approvers @tigrannajaryan @bogdandrutu

[codecov]

Codecov Report

❌ Patch coverage is 97.41935% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.47%. Comparing base (92c4252) to head (9a4eb4c).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
pdata/internal/generated_proto_anyvalue.go	92.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14546      +/-   ##
==========================================
+ Coverage   91.46%   91.47%   +0.01%     
==========================================
  Files         688      689       +1     
  Lines       43824    43975     +151     
==========================================
+ Hits        40084    40228     +144     
+ Misses       2629     2627       -2     
- Partials     1111     1120       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tigrannajaryan]

@florianl have you been able to run any benchmarks? I would be curious to see what's the end-to-end performance impact when using this approach vs the implementation that doesn't resolve references in a config with a bare otlp-receiver/otlp-exporter pipeline.

[tigrannajaryan]

ProfilesFromProto-20 9.317µ ± 5% 8.348µ ± 12% -10.40% (p=0.007 n=10)

@florianl can you please help interpret this? After the change the code is doing strictly more work if I understand correctly. So how can it it be 10% faster?

[florianl]

ProfilesFromProto-20 9.317µ ± 5% 8.348µ ± 12% -10.40% (p=0.007 n=10)

can you please help interpret this? After the change the code is doing strictly more work if I understand correctly.

‎BenchmarkProfilesToProto and BenchmarkProfilesFromProto are two existing benchmarks in the codebase. Both use the function generateBenchmarkProfiles that generates profiles with a given number of samples - no attributes, no resources, no dictionary entries. So in these cases no conversion is happening as there is no data that could be converted. I kept these two benchmarks as they are part of the codebase already and if the new code introduces a regression, I wanted to see that.

So how can it it be 10% faster?

My assumption is, that the compiler refactors the code and for the case where there are "empty" profiles - no attributes, no resources, no use of the dictionary - it performs better.
For that reason, I did write new benchmarks (see the gist), that populate resource attributes with a set of valid attributes, like semconv.ServiceNameKey, semconv.ServiceVersionKey, semconv.ProcessPIDKey, semconv.K8SPodNameKey, semconv.K8SNamespaceNameKey and semconv.TelemetrySDKNameKey. In these cases the benchmark use the new code to convert from/to reference based attributes.

@tigrannajaryan I hope this helps to put the benchmark results into perspective. I'm happy to add these new benchmarks to the codebase - please let me know.

[tigrannajaryan]

@tigrannajaryan I hope this helps to put the benchmark results into perspective. I'm happy to add these new benchmarks to the codebase - please let me know.

Yes, if you think the new benchmarks are a more valid comparison it would be good to see the results.

[florianl]

The results of these new benchmarks are shown in #14546 (comment) - see MarshalProfiles* and UnmarshalProfiles*. With fdab8d7 I have added them to the codebase - so they can fetch future regressions.

[tigrannajaryan]

The results of these new benchmarks are shown in #14546 (comment) - see MarshalProfiles* and UnmarshalProfiles*. With fdab8d7 I have added them to the codebase - so they can fetch future regressions.

Ahh, sorry I misunderstood the benchmarks.

So, from what I see there is virtually no penalty in the unmarshalling part and about 14% penalty in marshalling of small payloads. This seems acceptable to me.

My only concern would be to double check the benchmarks are correct. For example looking at MarshalProfiles, it shows no difference in allocs/op (actually only 1 alloc/op), but shouldn't this increase because we are building a map during marshalling? Is it possible benchmarks are not hitting the interesting paths? It would be great to benchmark with production data instead of synthetic data.

[tigrannajaryan]

@florianl in my opinion 14% marshalling speed penalty is an acceptable tradeoff to avoid the complexity of adding reference support to pdata API.

My only remaining concern is that I am not sure I understand why benchmarks don't show increase in memory allocations, which makes them suspect. If you can double check the benchmarks and explain what's going on then I am fully on board with this approach.

[dmathieu]

We should have a collector-contrib PR ready to be merged fixing the breaking changes.

[florianl]

@dmathieu the respective collector contrib PR is open-telemetry/opentelemetry-collector-contrib#46331