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 Sampler #622

Merged

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Aug 27, 2021

Hello 👋

I noticed that a few types like Resource and KeyValue implement Serialize and
Deserialize which is very handy for me because I can integrate the different
parameter into a configuration struct.

Unfortunately it is not yet implemented for Sampler so I had to implement it
on my crate but I think this could be beneficial for others.

Let me know what you think

@cecton cecton requested a review from a team as a code owner August 27, 2021 17:52
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 27, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #622 (a1d12e4) into main (a0899d9) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #622      +/-   ##
==========================================
- Coverage   62.08%   61.97%   -0.12%     
==========================================
  Files          95       95              
  Lines        7681     7681              
==========================================
- Hits         4769     4760       -9     
- Misses       2912     2921       +9     
Impacted Files Coverage Δ
opentelemetry/src/sdk/trace/sampler.rs 92.98% <0.00%> (+0.06%) ⬆️
opentelemetry-datadog/src/exporter/model/v03.rs 91.66% <0.00%> (-4.17%) ⬇️
opentelemetry-datadog/src/exporter/model/v05.rs 92.15% <0.00%> (-3.93%) ⬇️
opentelemetry-jaeger/src/exporter/collector.rs 67.85% <0.00%> (-3.58%) ⬇️
opentelemetry/src/global/error_handler.rs 42.10% <0.00%> (-2.34%) ⬇️
opentelemetry-jaeger/src/exporter/uploader.rs 24.00% <0.00%> (-2.09%) ⬇️
opentelemetry/src/metrics/registry.rs 49.41% <0.00%> (-1.20%) ⬇️
opentelemetry/src/sdk/trace/evicted_hash_map.rs 65.16% <0.00%> (-1.13%) ⬇️
opentelemetry-datadog/src/lib.rs 91.45% <0.00%> (-0.92%) ⬇️
opentelemetry/src/context.rs 92.06% <0.00%> (-0.80%) ⬇️
... and 24 more

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 a0899d9...a1d12e4. Read the comment docs.

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.

👍 Overall looks good. Nice job! The only thing we might want to examine is the current serialization/de-serialization process eliminated the ParentBased variant. I don't think it would cause any issue but I think the serialization process shouldn't change the variant.

@cecton
Copy link
Contributor Author

cecton commented Aug 29, 2021

omg what am I doing. Why do I even bother implementing them manually 🤯

Oh I know... I had in my own crate because I don't own the type. Then I copied here thinking I had too.

@TommyCpp this should be much easier to review. I'm not sure the test is useful at all. I suggest we delete it entirely.

@TommyCpp
Copy link
Contributor

Looks good. Thanks

@TommyCpp TommyCpp merged commit c02338a into open-telemetry:main Aug 29, 2021
@cecton cecton deleted the impl-serialize-deserialize-for-sampler branch August 30, 2021 08:43
@cecton
Copy link
Contributor Author

cecton commented Sep 1, 2021

Hey @TommyCpp I tried to lock the version on my side but it's actually very complicated. Because it's linked to tracing I have to fork tracing too and replace all the tracin dependencies in my project. Would it be possible for you to make a release?

@cecton
Copy link
Contributor Author

cecton commented Sep 1, 2021

@TommyCpp never mind, I just tried with the patch syntax instead of pinning the version in the dependency and it worked like a charm. The release can wait :)

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