Fix issue 6342: Automatically lowercase the index name #6640
Conversation
I clone the beats and fix issue 6342
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
| "strings" | ||
| ) | ||
|
|
||
| func TryLowercaseIndex(index string) string { |
There was a problem hiding this comment.
exported function TryLowercaseIndex should have comment or be unexported
There was a problem hiding this comment.
We probably don't need to export it :)
Hexilee
changed the title
| "strings" | ||
| ) | ||
|
|
||
| func TryLowercaseIndex(index string) string { |
There was a problem hiding this comment.
We probably don't need to export it :)
| } | ||
| } | ||
| return index | ||
| } |
There was a problem hiding this comment.
There is a few things that bug me in this:
- This could get noisy since it could be potentially be logged on every events.
- Worse case when the index is OK we iterate on all the chars, so an happy path will be hit by this.
- Due to the dynamic nature of index, we use string interpolation here, we have to do this check on every events.
I wonder if instead, we could provide a better errors message when this situation happen and either tell them to use a lowercase index or we could provide a lowercase processor.
There was a problem hiding this comment.
Could we provide a better errors message and an additional flag like --lowercase-index to enable the processor?
There was a problem hiding this comment.
Personnaly I would not add a custom flag, they are easy to add, hard to remove and it make the CLI more complicated.
But the error message could mention how to fix the situation.
"Use the lowercase processor to normalize your index"
There was a problem hiding this comment.
First of all I don't like magic. Sending a field with upper case and then converting it to lower case automatically for the index naming feels like magic to me. I see multiple options moving forward:
- A config option in the elasticsearch output to normalies / lowercase the index which is off by default.
- A lower case processor. The downside of the lower case processor is that if it's applied on a field, I expect it to be also changed in the final event it is sent.
I'm leaning towards the output config for the reason that this is an Elasticsearch specific issue so it should be an Elasticsearch config option. Having a config option off by default will also make it clear that when turning it on, it will have some processing overhead.
For the flag: As we have -E everything should be a config option and not a flag if possible (there are exceptions).
+1 on providing better error messages.
There was a problem hiding this comment.
A lower case processor. The downside of the lower case processor is that if it's applied on a field, I expect it to be also changed in the final event it is sent.
Correct.
Glad we got a conclusion.
I would be +1 to add a specific options to the elasticsearch output to lowercase the index name and make it off by default.
If you move forward with the changes can you sign the CLA?
There was a problem hiding this comment.
@ph Yeah, I'd like to, and I have signed the CLA.
However, as you say, current error message is generated by ElasticSearch, so should we improve ElasticSearch only? Whatever providing better error messages or lowercase processor.
kvch
added
the
needs CLA
ph
removed
the
needs CLA
|
sorry for the long delay seems there still a few comment open on this PR? Are you still interested in moving it forward? I am going to mark this PR as stalled and we will autoclose it. |
|
Closing this due to PR being stalled for quite some time. An alternative implementation is given in #16081 . |
Fix issue 6342