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

fix: fix how compress configuration flag works #90

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

andrzej-stencel
Copy link
Contributor

In v1.8.0, setting compress flag to either true or false caused compression to be enabled. This fixes multiple problems in the configure method of the plugin.

In v1.8.0, setting `compress` flag to either `true` or `false` caused compression to be enabled.
This fixes multiple problems in the `configure` method of the plugin.
@andrzej-stencel andrzej-stencel requested a review from a team as a code owner February 12, 2024 15:24
@sumo-drosiek
Copy link
Contributor

@astencel-sumo Could you explain what has been changed here? Why previous code didn't work?

@andrzej-stencel
Copy link
Contributor Author

andrzej-stencel commented Feb 13, 2024

@astencel-sumo Could you explain what has been changed here? Why previous code didn't work?

Sure.

  1. The conf map in the configure method contains only the values specified by user in configuration, in their raw string form. This means that if user hasn't specified e.g. compress in their config, the value of conf['compress'] is nil. In effect, the SumologicConnection.new method was being called with nils as parameter values for all config properties not explicitly specified by the user. This worked fine for when user did not specify compress when the default was false, but, due to the way the code in the SumologicConnection class is written and the dynamic nature of Ruby, this resulted in compression kicking in when user specified either true or false as the value of compress. This was because the value of the compress_enabled parametr to the SumologicConnection.initialize method was a string (either true or false), and when used in an if condition, it was evaluated as boolean true.
  2. The correct way to write code in the configure method is to use the instance variables like @compress, as described in Fluentd docs. Those variables are correctly initialized to either the value specified by the user (already parsed to the right type), or the default values specified in the plugin. This was not the case in our code, but was fixed by moving the invocation of the super method from the bottom of the configure method to its top (with the exception of the compat_parameters_convert method that goes before the super method).
  3. The other changes in the code and in the unit tests are a result of fixing those issues. For example, the code in the configure method checking the correctness of a conf['metrics_data_type'] was invalid - there's no such configuration option, there's metric_data_format instead. 🤷

@andrzej-stencel andrzej-stencel merged commit 0f8f5c8 into main Feb 13, 2024
1 check passed
@andrzej-stencel andrzej-stencel deleted the fix-configuration branch February 13, 2024 09:23
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.

3 participants