Skip to content

Fix Encoding/Escaping according to the InfluxDb Line-Protocol #55

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 3 commits into from
Mar 5, 2020
Merged

Fix Encoding/Escaping according to the InfluxDb Line-Protocol #55

merged 3 commits into from
Mar 5, 2020

Conversation

dangoodman
Copy link

@dangoodman dangoodman commented Mar 5, 2020

Description

This PR contains the following changes:

  1. Don't quote tags, since quotes become a part of it in this case.
  2. Pass rust ints as influxdb ints. In other case they are treated as floats.
  3. Other escaping rules according to the influxdb line protocol reference.

Tested it on ~1.2 million records during a conversion from mongodb. Better than nothing.

Fixes #52, closes #53

Checklist

  • Formatted code using cargo fmt --all
  • Linted code using clippy cargo clippy --all-targets --all-features -- -D warnings
  • Updated README.md using cargo readme > README.md
  • Reviewed the diff. Did you leave any print statements or unnecessary comments?
  • Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

@dangoodman
Copy link
Author

dangoodman commented Mar 5, 2020

Travis fails with

error[E0433]: failed to resolve: could not find `__rt` in `quote`
   --> /home/travis/.cargo/registry/src/github.1485827954.workers.dev-1ecc6299db9ec823/failure_derive-0.1.6/src/lib.rs:107:70
    |
107 | fn display_body(s: &synstructure::Structure) -> Result<Option<quote::__rt::TokenStream>, Error> {
    |                                                                      ^^^^ could not find `__rt` in `quote`

It doesn't seem to be related to changes in this PR. By the way, all tests are green, including the integration ones, at least on my PC. Do you have an idea how we can deal with it?

@Empty2k12
Copy link
Collaborator

Thanks for opening this PR 🚀

It doesn't seem to be related to changes in this PR. By the way, all tests are green, including the integration ones, at least on my PC. Do you have an idea how we can deal with it?

I found this resource from Rust announcements, apparently over 800 Crates are affected by this 😓

When the CI passes I will give this PR a thorough review and merge it. Am I seeing it right, that this should close #53?

@dangoodman
Copy link
Author

dangoodman commented Mar 5, 2020

Ah, ok. Hopefully will be fixed some day.

Well, this PR deals with spaces for sure.

\n's aren't supported by the line protocol. Might be a good idea to add a check for that.

\r and \t aren't mentioned in the docs. So needs more tests to figure out a way to handle them.

Copy link
Collaborator

@Empty2k12 Empty2k12 left a comment

Choose a reason for hiding this comment

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

Spotless and well written code with tests 👍

I'm fine with merging it like this, unless you want to add a check for line breaks and tabs, but since InfluxDb don't mention them, I don't think that's strictly necessary.

@Empty2k12 Empty2k12 changed the title Fix encoding/escaping according to the influxdb line protocol. Fix Encoding/Escaping according to the InfluxDb Line-Protocol Mar 5, 2020
@dangoodman
Copy link
Author

Thank you for the fast response. I will test how it works with \r's and \t's once have time, maybe on the next week, and file a new PR if it comes.

@Empty2k12 Empty2k12 merged commit 90f5f95 into influxdb-rs:master Mar 5, 2020
@Empty2k12
Copy link
Collaborator

Thank you, feel free to open more issues for any issues you encounter or features you're missing from the library :)

@satirebird
Copy link

I tested the PR and it works as expected. Thanks a lot for the work. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spaces and double quotes
3 participants