Skip to content
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

Remove "Attribute" part from common pdata collections names #5001

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Mar 15, 2022

All of the pdata wrappers for collections include Attribute part in its name because the fields used to be part of the attributes fields only. But it's not the case anymore since pdata.LogRecord.Body uses AnyValue that can contain any common collection. This change renames AttributeMap and AttributeValueSlice collections by removing Attribute part from their names and making them consistent.

Renames:

  • Deprecate pdata.AttributeMap struct in favor of pdata.Map
  • Deprecate pdata.NewAttributeMap func in favor of pdata.NewMap
  • Deprecate pdata.NewAttributeMapFromMap func in favor of pdata.NewMapFromRaw
  • Deprecate pdata.AttributeValueSlice struct in favor of pdata.Slice
  • Deprecate pdata.NewAttributeValueSlice func in favor of pdata.NewSlice

Link to tracking Issue: #4988

@dmitryax dmitryax requested review from a team and jpkrohling March 15, 2022 04:59
@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #5001 (2c22ea4) into main (510dbab) will decrease coverage by 0.02%.
The diff coverage is 94.80%.

@@            Coverage Diff             @@
##             main    #5001      +/-   ##
==========================================
- Coverage   91.08%   91.05%   -0.03%     
==========================================
  Files         180      180              
  Lines       10700    10788      +88     
==========================================
+ Hits         9746     9823      +77     
- Misses        737      747      +10     
- Partials      217      218       +1     
Impacted Files Coverage Δ
model/internal/pdata/common.go 94.26% <93.50%> (-1.27%) ⬇️
internal/otlptext/databuffer.go 100.00% <100.00%> (ø)
internal/otlptext/logs.go 100.00% <100.00%> (ø)
internal/otlptext/metrics.go 100.00% <100.00%> (ø)
internal/otlptext/traces.go 100.00% <100.00%> (ø)
model/internal/pdata/generated_common.go 100.00% <100.00%> (ø)
model/internal/pdata/generated_log.go 96.65% <100.00%> (ø)
model/internal/pdata/generated_metrics.go 97.14% <100.00%> (ø)
model/internal/pdata/generated_resource.go 100.00% <100.00%> (ø)
model/internal/pdata/generated_trace.go 96.89% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 510dbab...2c22ea4. Read the comment docs.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Just a nit, otherwise it looks good.

model/internal/pdata/common.go Outdated Show resolved Hide resolved
@dmitryax dmitryax force-pushed the rename-map-slice branch 5 times, most recently from d664c69 to 09fef01 Compare March 15, 2022 19:53
Comment on lines +59 to +115
NewMap = pdata.NewMap
NewMapFromRaw = pdata.NewMapFromRaw
Copy link
Member

Choose a reason for hiding this comment

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

nit: separate PR, I would prefer to rename internal/pdata to maybe just internal, I am confused about the 2 packages named the same, when code refers to one vs the other.

@bogdandrutu
Copy link
Member

Please rebase

@dmitryax dmitryax force-pushed the rename-map-slice branch 11 times, most recently from e9763d3 to 5054199 Compare March 17, 2022 08:41
model/internal/pdata/common.go Show resolved Hide resolved
model/internal/pdata/common.go Show resolved Hide resolved
@dmitryax dmitryax force-pushed the rename-map-slice branch 2 times, most recently from e752665 to 87b96f4 Compare March 17, 2022 18:38
@dmitryax dmitryax force-pushed the rename-map-slice branch 2 times, most recently from e38b217 to df3cd35 Compare March 20, 2022 17:34
model/internal/pdata/common.go Outdated Show resolved Hide resolved
model/internal/pdata/common.go Outdated Show resolved Hide resolved
All of the pdata wrappers for collections include Attribute part in its name because the fields used to be part of the attributes fields only. But it's not the case anymore since `pdata.LogRecord.Body` uses AnyValue that can contain any common collection. This change renames AttributeMap and AttributeValueSlice collections by removing Attribute part from their names and making them consistent.
@bogdandrutu bogdandrutu merged commit d6b6251 into open-telemetry:main Mar 21, 2022
@dmitryax dmitryax deleted the rename-map-slice branch March 23, 2022 06:31
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
…emetry#5001)

* Remove "Attribute" part from common pdata collections names

All of the pdata wrappers for collections include Attribute part in its name because the fields used to be part of the attributes fields only. But it's not the case anymore since `pdata.LogRecord.Body` uses AnyValue that can contain any common collection. This change renames AttributeMap and AttributeValueSlice collections by removing Attribute part from their names and making them consistent.

* Change NewMapFromRaw to take interface{}

Co-authored-by: Bogdan Drutu <[email protected]>
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.

3 participants