Skip to content

Add FromAttributeIndices helper to pprofile #12176

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

Merged

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented Jan 23, 2025

Description

Profiles attributes are stored in a single AttributeTable attribute, and then referenced in each substruct as AttributeIndices.

This means to read them as a pcommon.Map, a bit of pre-processing is required.
This adds an helper method so each component manipulating profiles doesn't have to reimplement it.

Benchmark:

go test -bench=BenchmarkFromAttributeIndices ./...
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/collector/pdata/pprofile
cpu: Apple M1 Max
BenchmarkFromAttributeIndices-10         9789714               123.2 ns/op           140 B/op          4 allocs/op
PASS
ok      go.opentelemetry.io/collector/pdata/pprofile    1.952s
PASS
ok      go.opentelemetry.io/collector/pdata/pprofile/pprofileotlp       0.407s

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.85%. Comparing base (d977da4) to head (921f92f).
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12176   +/-   ##
=======================================
  Coverage   91.84%   91.85%           
=======================================
  Files         465      466    +1     
  Lines       25325    25334    +9     
=======================================
+ Hits        23261    23270    +9     
  Misses       1675     1675           
  Partials      389      389           

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

@dmathieu dmathieu marked this pull request as ready for review January 23, 2025 17:08
@dmathieu dmathieu requested review from mx-psi and a team as code owners January 23, 2025 17:08
@dmathieu dmathieu marked this pull request as draft January 23, 2025 17:26
@dmathieu dmathieu force-pushed the pprofile-build-attributes-helper branch from 4dc8a4a to e0a0548 Compare January 24, 2025 08:33
@dmathieu dmathieu marked this pull request as ready for review January 24, 2025 08:41
@dmathieu dmathieu requested a review from bogdandrutu January 24, 2025 08:41
@bogdandrutu bogdandrutu dismissed their stale review January 24, 2025 16:53

The blocking request part is resolved.

@dmathieu dmathieu force-pushed the pprofile-build-attributes-helper branch from c8f7bfa to 35f5bfd Compare January 27, 2025 08:39
@dmathieu dmathieu changed the title Add BuildAttributes helper to pprofile Add FromAttributeIndices helper to pprofile Jan 27, 2025
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me in general, would like other to specify where this function should live.

Couple things:

  1. Did you update benchmarks after latest changes?
  2. We should document that changes to the return map are not reflected to the profile. Would like to understand how you think we should design the API that can do changes.

@dmathieu dmathieu force-pushed the pprofile-build-attributes-helper branch from 6a872fd to eef33eb Compare January 28, 2025 07:45
@dmathieu
Copy link
Member Author

Did you update benchmarks after latest changes?

Sorry, they're updated now.

We should document that changes to the return map are not reflected to the profile. Would like to understand how you think we should design the API that can do changes.

I've added a mention that updates on the map will not change the record.
Making changes is a bit tricky, as we have to check the AttributeTable to find existing entries, possibly add new ones.
And we won't be able to remove data that's not used anymore, as we can't know it's not used by other records (that shouldn't matter much though).

We could have something like:

ToAttributeIndices(data pcommon.Map, table AttributeTable, record attributable)

That would properly update the AttributeTable with new entries, as well as the attributable indices.
That would be a full replace. So we could also have a single attribute add:

Addattribute(key, value string, table AttributeTable, record attributable)

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good. But I want you to add an issue and mark it as blocker for stabilizing profiles to find a solution for updates and try it before stabilizing this package.

@bogdandrutu
Copy link
Member

Sorry, they're updated now.

Numbers look much better than at the beginning 👯

@bogdandrutu bogdandrutu added this pull request to the merge queue Jan 31, 2025
Merged via the queue into open-telemetry:main with commit 8de90e7 Jan 31, 2025
53 checks passed
@dmathieu dmathieu deleted the pprofile-build-attributes-helper branch January 31, 2025 08:27
sfc-gh-sili pushed a commit to sfc-gh-sili/opentelemetry-collector that referenced this pull request Feb 3, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Profiles attributes are stored in a single `AttributeTable` attribute,
and then referenced in each substruct as `AttributeIndices`.

This means to read them as a `pcommon.Map`, a bit of pre-processing is
required.
This adds an helper method so each component manipulating profiles
doesn't have to reimplement it.

Benchmark:

```
go test -bench=BenchmarkFromAttributeIndices ./...
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/collector/pdata/pprofile
cpu: Apple M1 Max
BenchmarkFromAttributeIndices-10         9789714               123.2 ns/op           140 B/op          4 allocs/op
PASS
ok      go.opentelemetry.io/collector/pdata/pprofile    1.952s
PASS
ok      go.opentelemetry.io/collector/pdata/pprofile/pprofileotlp       0.407s
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants