-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Elasticsearch bulk request enhancements #5811
Conversation
eecce76
to
5bcc5c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a really nice addition. I left a few minor comments.
meta := createEventBulkMeta(index, pipeline, event) | ||
meta, err := createEventBulkMeta(index, pipeline, event) | ||
if err != nil { | ||
logp.Err("Failed to encode event meta dat: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit s/dat/data
Pipeline string `json:"pipeline" struct:"pipeline"` | ||
var id interface{} | ||
if m := event.Meta; m != nil { | ||
id = m["id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to other PR: Should we really allow here interface or make sure this is converted to a string? Do we know the key "id"
exists in m
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can _id
be a number instead of a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that _id
on the ES side is a keyword. So if 12
is put in it ends up also as "12"
. Even if that is not the case, I think we should enforce string values on beats side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@urso Needs a rebase. |
- Log error and drop event if no index name can be computed (ES API returns error if index is empty) - Set `_id` if event.Meta["id"] is set - Use `index` action if no `id` is set and `create` if `id` is set.
933da88
to
93aaea6
Compare
libbeat/beat/event.go
Outdated
@@ -22,6 +22,13 @@ var ( | |||
errNoTimestamp = errors.New("value is no timestamp") | |||
) | |||
|
|||
func (e *Event) SetID(id string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported method Event.SetID should have comment or be unexported
@ruflin rebased and ensure |
Add support to configure a key for setting the document ID in the harvester JSON settings. The ID will be store in the events Meta["id"] for the output to pick up. With elastic#5811 will the elasticsearch output is the Meta["id"] field to set the document its ID (uses op_type="create" to count duplicate inserts of same ID). For other output type, the document ID will be forwarded via `@metadata.id`.
* Add support to set the document ID in the filebeat json reader Add support to configure a key for setting the document ID in the harvester JSON settings. The ID will be store in the events Meta["id"] for the output to pick up. With #5811 will the elasticsearch output is the Meta["id"] field to set the document its ID (uses op_type="create" to count duplicate inserts of same ID). For other output type, the document ID will be forwarded via `@metadata.id`.
Requires: #5810
returns error if index is empty)
_id
if event.Meta["id"] is setindex
action if noid
is set andcreate
ifid
is set.