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

Added MarshalerSizer Interface to facilitate creation of Sizer implementations #5929

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

cpheps
Copy link
Contributor

@cpheps cpheps commented Aug 17, 2022

Description:
Proposing adding a new MarshalerSizer interface to ptrace, plog, and pmetric packages and alter NewProtoMarshaler to return this interface. This allows easy creation of a Sizer implementation. Currently the only way to create a sizer for the pdata packages is to call NewProtoMarshaler function then cast to a Sizer. This is not intuitive from an API standpoint that it should be safe to cast a Marshaler as a Sizer.

The MarshalerSizer interface is a combination of the Marshaler and Sizer interfaces.

If this is merged I will create a follow up PR to update the Batch Processor to use this rather than casting from a Marshaler.

Link to tracking Issue: #5920

Testing: Updated tests to use the MarshalSizer instead of casting.

@cpheps cpheps requested review from a team and bogdandrutu August 17, 2022 17:04
pdata/plog/pb.go Outdated Show resolved Hide resolved
@cpheps cpheps changed the title Added NewProtoSizer to instantiate Sizer implementations Added MarshalSizer Interface to facilitate creation of Sizer implementations Aug 17, 2022
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #5929 (e9ddacb) into main (e0adb9d) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e9ddacb differs from pull request most recent head 42e8441. Consider uploading reports for the commit 42e8441 to get more accurate results

@@           Coverage Diff           @@
##             main    #5929   +/-   ##
=======================================
  Coverage   91.80%   91.80%           
=======================================
  Files         198      198           
  Lines       12377    12377           
=======================================
  Hits        11363    11363           
  Misses        800      800           
  Partials      214      214           
Impacted Files Coverage Δ
pdata/plog/pb.go 100.00% <100.00%> (ø)
pdata/pmetric/pb.go 100.00% <100.00%> (ø)
pdata/ptrace/pb.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #5929 (e953488) into main (109c917) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5929   +/-   ##
=======================================
  Coverage   91.93%   91.93%           
=======================================
  Files         198      198           
  Lines       12417    12417           
=======================================
  Hits        11415    11415           
  Misses        791      791           
  Partials      211      211           
Impacted Files Coverage Δ
pdata/plog/pb.go 100.00% <100.00%> (ø)
pdata/pmetric/pb.go 100.00% <100.00%> (ø)
pdata/ptrace/pb.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

I expect to remove some of the conversions to Sizer from batch processor, do I miss something and that is not possible?

pdata/plog/encoding.go Outdated Show resolved Hide resolved
Corbin Phelps added 5 commits August 18, 2022 08:31
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
…ProtoMarshaller

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
@cpheps cpheps force-pushed the new-sizer-functions branch from 42e8441 to e953488 Compare August 18, 2022 12:31
@cpheps
Copy link
Contributor Author

cpheps commented Aug 18, 2022

I expect to remove some of the conversions to Sizer from batch processor, do I miss something and that is not possible?

@bogdandrutu I wasn't sure if that should be part of this PR or not. This changes makes the current logic still valid. I was going to do a follow up PR with the batch processor changes but I can group them in here if you'd rather just have a single PR.

@cpheps cpheps changed the title Added MarshalSizer Interface to facilitate creation of Sizer implementations Added MarshalerSizer Interface to facilitate creation of Sizer implementations Aug 18, 2022
@bogdandrutu
Copy link
Member

@cpheps sounds good, but still a question in the issue if you can respond

@bogdandrutu bogdandrutu merged commit 22ca5e0 into open-telemetry:main Aug 18, 2022
bogdandrutu pushed a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Aug 18, 2022
…entations (open-telemetry#5929)

* Added NewProtoSizer to instantiate Sizer implementations

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Updated changelog with PR number

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Refactored to create a MarshalSizer interface that is returned by NewProtoMarshaller

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Updated NewProtoMarshaler comments to inidicate return interface

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Corrected interface name

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
bogdandrutu added a commit that referenced this pull request Aug 22, 2022
…entations (#5929) (#5931)

* Added NewProtoSizer to instantiate Sizer implementations

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Updated changelog with PR number

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Refactored to create a MarshalSizer interface that is returned by NewProtoMarshaller

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Updated NewProtoMarshaler comments to inidicate return interface

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Corrected interface name

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Co-authored-by: Corbin Phelps <corbin.phelps@bluemedora.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Sep 6, 2022
In open-telemetry#5929, the suggestion to have the combined name as "MarshalerSizer" was wrong.

Even the comment open-telemetry#5929 (comment) was wrong since the link is `WriteSeeker`.

Signed-off-by: Bogdan <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Sep 6, 2022
In open-telemetry#5929, the suggestion to have the combined name as "MarshalerSizer" was wrong.

Even the comment open-telemetry#5929 (comment) was wrong since the link is `WriteSeeker`.

Signed-off-by: Bogdan <bogdandrutu@gmail.com>
bogdandrutu added a commit that referenced this pull request Sep 6, 2022
In #5929, the suggestion to have the combined name as "MarshalerSizer" was wrong.

Even the comment #5929 (comment) was wrong since the link is `WriteSeeker`.

Signed-off-by: Bogdan <bogdandrutu@gmail.com>

Signed-off-by: Bogdan <bogdandrutu@gmail.com>
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