Skip to content

Add base64 conversion traits #81

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

Merged
merged 5 commits into from
Mar 21, 2020
Merged

Conversation

xd009642
Copy link
Contributor

I'll add some tests and more doc comments later just wanted to make sure this was in the ballpark of what was expected.

Fixes #54

@xd009642 xd009642 requested a review from a team March 19, 2020 23:17
@jtescher
Copy link
Member

@xd009642 yeah this direction looks good, maybe have this be an optional feature like serde is currently? Nice to keep the dependency list short for people who don't need all the features.

@xd009642
Copy link
Contributor Author

I can do that once I finish work today 👍 base64 is a small dependency though with no subdependencies so should I at least make it a default feature?

@xd009642
Copy link
Contributor Author

also I signed the cla/linuxfoundation so I guess that will switch to pass on the next commit I push?

@jtescher
Copy link
Member

Sounds good! Normally I think default feature could make sense, however the binary format is in flux / may be removed so probably best to have this be opt-in for now.

Starting work on tests
@xd009642
Copy link
Contributor Author

@jtescher I've moved base64 to it's own feature and also decided to do a blanket implementation for anything that implements BinaryFormat filled in the to_base64 test replicating the binary format to_bytes test. Just need to fill in some more tests for the from_base64

@xd009642
Copy link
Contributor Author

I signed it

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

This looks good! do you want to also export this in the api module to make it a little more ergonomic for consumers? nice to be able to use at that level: use api::Base64Format

@xd009642
Copy link
Contributor Author

Done 😄

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@jtescher jtescher merged commit 07ad1e6 into open-telemetry:master Mar 21, 2020
@xd009642 xd009642 deleted the base64 branch March 21, 2020 18:42
cijothomas pushed a commit to cijothomas/opentelemetry-rust that referenced this pull request Sep 7, 2024
scottgerring pushed a commit to scottgerring/opentelemetry-rust that referenced this pull request Apr 2, 2025
## Motivation

As described in open-telemetry#81, currently the metrics layer doesn't do anything
when Debug / Display values are given.

## Solution

Record `Debug` attributes as otel metric attributes.
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.

Add a Base64 decoder
2 participants