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

Implement Serialize & Deserialize for more stuff #626

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Sep 1, 2021

Same than #622 but for:

  • SpanLimits
  • Protocol
  • Credentials (also re-export to make it public)
  • Compression (also re-export to make it public)

@cecton cecton requested a review from a team as a code owner September 1, 2021 13:01
@cecton cecton force-pushed the impl-serialize-deserialize-for-span-limits branch from 52dd6f9 to c0bf04b Compare September 1, 2021 13:01
@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #626 (353e54b) into main (c02338a) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
- Coverage   61.97%   61.96%   -0.01%     
==========================================
  Files          95       95              
  Lines        7681     7682       +1     
==========================================
  Hits         4760     4760              
- Misses       2921     2922       +1     
Impacted Files Coverage Δ
opentelemetry/src/sdk/trace/span_limit.rs 30.00% <0.00%> (-3.34%) ⬇️

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 c02338a...353e54b. Read the comment docs.

@cecton cecton marked this pull request as draft September 1, 2021 13:20
@cecton cecton changed the title Implement Serialize & Deserialize for SpanLimits Implement Serialize & Deserialize for more stuff Sep 1, 2021
@@ -214,12 +214,15 @@ pub use crate::exporter::{
use opentelemetry::sdk::export::ExportError;

#[cfg(feature = "grpc-sys")]
pub use crate::exporter::grpcio::GrpcioExporterBuilder;
pub use crate::exporter::grpcio::{Compression, Credentials, GrpcioExporterBuilder};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few things to say here:

  1. They should probably be prefixed with Grpcio as they are specific to that exporter
  2. They were public but not accessible. There was no way to call with_compression or with_credentials because the types couldn't be instantiated. In the doc you can see it because the types are not clickable:
    image

@cecton cecton marked this pull request as ready for review September 1, 2021 15:26
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks.

@TommyCpp TommyCpp merged commit 414b3f4 into open-telemetry:main Sep 3, 2021
@cecton cecton deleted the impl-serialize-deserialize-for-span-limits branch September 3, 2021 09:10
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.

None yet

2 participants