-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add stringSlice and int64Slice to pcommon, and let tests handle non-number slices #10148
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10148 +/- ##
==========================================
- Coverage 92.24% 92.15% -0.10%
==========================================
Files 353 357 +4
Lines 16797 16918 +121
==========================================
+ Hits 15495 15591 +96
- Misses 963 988 +25
Partials 339 339 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should not be a controversial addition, but since this is adding new API on a 1.0 module I would like other @open-telemetry/collector-approvers to take a look.
AIUI the motivation is to support fields like https://github.com/open-telemetry/opentelemetry-proto/blob/a05597bff803d3d9405fcdd1e1fb1f42bed4eb7a/opentelemetry/proto/profiles/v1experimental/pprofextended.proto#L97, so the risk is that we may not use this after all (sounds very unlikely though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a problem adding these, but want to make sure there is no other way for us to support this (if you have any idea, it would be good to brainstorm here).
@dmathieu To confirm: the intent is to use these types for fields like |
Yes, it is. My pdata PR shows that use as well. |
Thanks for confirming. I will wait until Friday before merging this to give time to others to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at #10109 I only see the new types being used in pprofile
. Is it required that the new types be in pcommon
?
IMO it is required after profiling is declared stable. It's not strictly required now, but it's better in that we don't need people to update their import paths for this after profiling goes to 1.0 |
I guess the risk is that this goes into 1.x pcommon and then needs removed/modified. Unlikely, but how would we handle that? Deprecate it and remove in 2.0? |
Yup, it would become dead code |
What would we do if it needs to be modified instead of removed? |
We would have to deprecate it and come up with a different name. I don't think that's going to happen here though, what a |
Waiting for v0.101.0 to be released, will merge after that |
…umber slices (open-telemetry#10148) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This adds a string slice to pcommon, that will be used by the profiles pdata. #### Link to tracking issue Part of open-telemetry#10109. #### Testing This is unit-tested. cc @mx-psi --------- Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
…umber slices (open-telemetry#10148) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This adds a string slice to pcommon, that will be used by the profiles pdata. #### Link to tracking issue Part of open-telemetry#10109. #### Testing This is unit-tested. cc @mx-psi --------- Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Description
This adds a string slice to pcommon, that will be used by the profiles pdata.
Link to tracking issue
Part of #10109.
Testing
This is unit-tested.
cc @mx-psi