opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Add stringSlice and int64Slice to pcommon, and let tests handle non-number slices

Open dmathieu opened this issue 1 year ago • 5 comments

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

dmathieu avatar May 14 '24 07:05 dmathieu

Codecov Report

Attention: Patch coverage is 79.33884% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 92.15%. Comparing base (e7dcfcc) to head (83f82f1).

Files Patch % Lines
pdata/internal/generated_wrapper_byteslice.go 0.00% 5 Missing :warning:
pdata/internal/generated_wrapper_float64slice.go 0.00% 5 Missing :warning:
pdata/internal/generated_wrapper_int64slice.go 54.54% 5 Missing :warning:
pdata/internal/generated_wrapper_stringslice.go 54.54% 5 Missing :warning:
pdata/internal/generated_wrapper_uint64slice.go 0.00% 5 Missing :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 14 '24 07:05 codecov[bot]

@dmathieu To confirm: the intent is to use these types for fields like string_table or location_indices, right?

mx-psi avatar May 15 '24 18:05 mx-psi

Yes, it is. My pdata PR shows that use as well.

dmathieu avatar May 15 '24 18:05 dmathieu

Thanks for confirming. I will wait until Friday before merging this to give time to others to review

mx-psi avatar May 15 '24 18:05 mx-psi

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

mx-psi avatar May 16 '24 21:05 mx-psi

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?

TylerHelmuth avatar May 17 '24 15:05 TylerHelmuth

Deprecate it and remove in 2.0?

Yup, it would become dead code

mx-psi avatar May 17 '24 15:05 mx-psi

What would we do if it needs to be modified instead of removed?

TylerHelmuth avatar May 17 '24 17:05 TylerHelmuth

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 StringSlice type should look like is kind of already defined by the other <Type here>Slice types we already support

mx-psi avatar May 17 '24 17:05 mx-psi

Waiting for v0.101.0 to be released, will merge after that

mx-psi avatar May 21 '24 09:05 mx-psi