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

Add encoding selector option (base64 or binary) #39

Merged
merged 7 commits into from
Apr 28, 2022

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Apr 15, 2022

Release notes

Add encoding option to select the encoding of Avro payload between base64 (default) and binary

What does this PR do?

Adds encoding set option to choose the encoding of Avro payload.
By default it's set to base64.

Why is it important/What is the impact to the user?

Let the user to decide whether to use binary or base64 encoding.

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • run this gem in Logstash

How to test this PR locally

  • clone this branch and configure Gemfile
  • execute the pipeline:
input {
  generator {
    message => '{"id": "my account", "amount": 5.0}'
    count => 1 
    codec => json
  }
}

output {
  stdout {
    codec => avro {
      schema_uri => "/tmp/avro_schema_payment.asvc"
      base64_encoding => false
    }
  }
}
  • Avro schema to save in /tmp/avro_schema_payment.asvc
{"namespace": "io.confluent.examples.clients.basicavro",
 "type": "record",
 "name": "Payment",
 "fields": [
     {"name": "id", "type": "string"},
     {"name": "amount", "type": "double"}
 ]
}

Related issues

Use cases

As user I want to receive the binary Avro payload without base64 encoding.

@andsel andsel self-assigned this Apr 15, 2022
@andsel
Copy link
Contributor Author

andsel commented Apr 15, 2022

I would ask if @karenzone could give a quick check to the documentation change

@andsel andsel force-pushed the fix/add_base64_encoding_flag branch from 94f4a66 to d8ae933 Compare April 15, 2022 14:37
@@ -99,6 +100,16 @@ output {

Controls this plugin's compatibility with the {ecs-ref}[Elastic Common Schema (ECS)].

[id="plugins-{type}s-{plugin}-base64_encoding"]
===== `base64_encoding`
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer we avoid adding boolean options tied to the current implementation detail of there being two options, instead opting for a name that is descriptive of what the option controls and a value that is descriptive of what is being done. This allows us to leave room for additional options in the future, such as alternate encodings that may be introduced upstream.

In this case, something like:

encoding with values base64 and binary.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on previous comment. If you take this suggestion, the doc will need to be reworded. Please ping me and I'll review/retest doc then. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggesting it. I've applied the hint, so it's ready for a review :-)

@andsel andsel requested review from karenzone and yaauie April 19, 2022 09:05
@andsel andsel changed the title Fix/add base64 encoding flag Add encoding selector option (base64 or binary) Apr 19, 2022
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

I left a suggestion inline to streamline and simplify the wording. Otherwise, LGTM

docs/index.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

In general, LGTM. I've left two optional nitpick suggestions.

lib/logstash/codecs/avro.rb Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
@andsel andsel merged commit 019a5ee into logstash-plugins:main Apr 28, 2022
@andsel
Copy link
Contributor Author

andsel commented Apr 28, 2022

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.

5 participants