feat: Update Elasticsearch json parser to use LoggingReceiverMacro#1987
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
...erator/testdata/goldens/logging-receiver_elasticsearch/golden/linux-gpu/fluent_bit_main.conf
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Note: This is a general comment that i mentioned also in #1978 (comment). Another way of verifying that the refactor is correct is that the output_fluentbit.yaml should be the same before and after the update to elasticsearch.go.
65124b7 to
b16bece
Compare
franciscovalentecastro
left a comment
There was a problem hiding this comment.
Sorry for bringing this comments so late in the review process. Added some last suggested changes.
apps/elasticsearch.go
Outdated
| ReceiverMixin confgenerator.LoggingReceiverFilesMixin `yaml:",inline"` | ||
| type LoggingReceiverMacroElasticsearchJson struct { | ||
| LoggingProcessorMacroElasticsearchJson `yaml:",inline"` | ||
| ReceiverMixin confgenerator.LoggingReceiverFilesMixin `yaml:",inline"` |
There was a problem hiding this comment.
This change should prevent the YAML un-marshalling to validate recursively all fields in ReceiverMixin, thus not causing the panic with missing yaml tags.
Suggested change :
| ReceiverMixin confgenerator.LoggingReceiverFilesMixin `yaml:",inline"` | |
| ReceiverMixin confgenerator.LoggingReceiverFilesMixin `yaml:",inline" validate:"structonly"` |
confgenerator/logging_processors.go
Outdated
| StateName string `yaml:"state_name,omitempty"` | ||
| Regex string `yaml:"regex,omitempty"` | ||
| NextState string `yaml:"next_state,omitempty"` |
There was a problem hiding this comment.
Sorry for bringing this comments so late in the review process. Now i'm realizing that since MultilineRule 1 is a non-configurable feature (not exposed to users), these fields don't need and shouldn't have named yaml tags.
Note : I remember the updates in elasticsearch.go were causing some issues with a panic (missing tags) when running tests. I haven't been able to reproduce again, but i believe the other suggested change in elasticsearch.go should fix that.
Suggesting the following change :
| StateName string `yaml:"state_name,omitempty"` | |
| Regex string `yaml:"regex,omitempty"` | |
| NextState string `yaml:"next_state,omitempty"` | |
| StateName string | |
| Regex string | |
| NextState string |
Footnotes
All good, got that fixed! |
|
Previous commit passed most tests (see a483257). Merging. |
308261b
into
GoogleCloudPlatform:master
Description
Refactored the Elasticsearch json parser to use the
LoggingReceiverMacroI did need to add some yaml tags to fix a parsing panic
Related issue
How has this been tested?
Checklist: