-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: support multiple versions of Elasticsearch logger #12364
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
Conversation
headers["Content-Type"] = headers["Content-Type"] .. compat_header_8 | ||
headers["Accept"] = headers["Accept"] .. compat_header_8 | ||
if conf.field.type then | ||
core.log.warn("type is not supported in Elasticsearch 9, removing `type`") |
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 think we should use error log level
@@ -195,6 +264,11 @@ function _M.body_filter(conf, ctx) | |||
log_util.collect_body(conf, ctx) | |||
end | |||
|
|||
function _M.access(conf) |
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.
set_version calls ES server the first time and this http connection is not allowed in log phase so I had no other way but to add this here because version information is needed at the start of log phase when entries are being created.
} | ||
if conf._version == "8" then | ||
headers["Content-Type"] = headers["Content-Type"] .. compat_header_7 |
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 we remove compat_header_7
and compat_header_8
?
If yes, then we can move if conf._version == "8" then
and elseif conf._version == "9" then
.
just use the default header, pls confirm if it is ok:
local headers = {
["Content-Type"] = "application/x-ndjson",
["Accept"] = "application/vnd.elasticsearch+json"
}
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 do not like if conf._version == "8" then ... elseif conf._version == "9" then ... end
style
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.
removed it. not needed anymore
function _M.access(conf) | ||
-- set_version will call ES server only the first time | ||
-- so this should not amount to considerable overhead | ||
set_version(conf) |
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.
how about fetch_and_update_es_version
?
core.json.encode(entry) .. "\n" | ||
end | ||
|
||
local function set_version(conf) |
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.
local function set_version(conf)
if conf._version then
return
end
local selected_endpoint_addr
if conf.endpoint_addr then
selected_endpoint_addr = conf.endpoint_addr
else
selected_endpoint_addr = conf.endpoint_addrs[math_random(#conf.endpoint_addrs)]
end
local major_version, err = get_es_major_version(selected_endpoint_addr, conf)
if err then
return false, str_format("failed to get Elasticsearch version: %s", err)
end
conf._version = major_version
end
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.
done
end | ||
local major_version, err = get_es_major_version(selected_endpoint_addr, conf) | ||
if err then | ||
return false, str_format("failed to get Elasticsearch version: %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.
we should write error log here
we do not capture the error msg in access
phase
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.
done
xpack.security.enabled: 'true' | ||
|
||
elasticsearch-auth-4: | ||
image: docker.elastic.co/elasticsearch/elasticsearch:6.7.0 |
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.
we started more instances of es
the github action, is it power enough to support this? I am little worried
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 removed two of these containers(not needed)
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.
Do we need to remove type
field in this PR?
Yes. If we don't remove |
pls update the doc too |
Removed |
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.
LGTM
Fixes

Error comes when using elasticsearch version 9+
We want to remove the compat header and have a general support for all major versions.
For elasticsearch logger plugin comptability check. Following are the observations and conclusion based on reading docs and self testing.
The
type
field issue:Before version 7, the
type
field was required in json schema https://github.com/elastic/elasticsearch/blob/8453f7701de25c467ce08088ddddac185cb8028c/rest-api-spec/src/main/resources/rest-api-spec/api/create.json#L21.The current tests without
type
fail on version 6(here it's 6.7) with the following errorBatch Processor[elasticsearch-logger] failed to process entries: elasticsearch server returned status: 400, body: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: type is missing;"}]
Adding a _type in the code fixes the above test.
Conclusion
As suggested in the migration guide, we can hardcode the
type
to "_doc" in versions prior to 7 and on 7+ we can omittype
completelyIndices created in 6.x only allow a single-type per index. Any name can be used for the type, but there can be only one. The preferred type name is _doc, so that index APIs have the same path as they will have in 7.0: PUT {index}/_doc/{id} and POST {index}/_doc
The
id
field issue:Another problem was detected while running tests on older version(6), when _id was not passed along with _index,. the following error was reported:
Batch Processor[elasticsearch-logger] failed to process entries: elasticsearch server returned status: 400, body: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: an id must be provided if version type or value are set;"}],"
In older version, _id is also required https://github.com/elastic/elasticsearch/blob/8453f7701de25c467ce08088ddddac185cb8028c/rest-api-spec/src/main/resources/rest-api-spec/api/create.json#L9
But I when I used index API instead of create API, request passed even without _id for all versions 6,7,8
The doc says
A create action fails if a document with the same ID already exists in the target An index action adds or replaces a document as necessary.
An answer in the discuss forum says
Neither is used to create an index (although they may do so as a side-effect of inserting the first document into a new index). "index" indexes a document. "create" only indexes a document if it doesn't already exist. It used to be that "create" was faster if you knew that your docs didn't already exist. But now, since versioning was added, "index" performs as well as "create"
Another answer suggests
you use create if a doc with that id already exists, if you use index it will create a document or update it if xisting.
Conclusion: To me it looks okay to replace create with index
Conclusion:
We can remove
type
from plugin configuration and for detected version below 7, hardcode it as_doc
We can replace Create with Index in the request body.
We run all tests on version 8 and my PR adds a comptability test with version 9. We can add two more compatibilty tests with 6 and 7 to confirm our changes.
Result: We will be able to support ES versions 6,7,8,9
References and other relevant information:
https://www.elastic.co/guide/en/elasticsearch/reference/6.7/release-notes-6.7.0.html. Search "Deprecate types" in this doc.
From reading this issue: elastic/elasticsearch#35190
Though a temporary parameter was added
include_type_name
which defaults to true between 6.7 upto 7 meaning by defaulttype
was being sent but it could be turned off.Then between 7 and 8 this field was default to false meaning by default the
type
will no longer be sent and a deprecated warning will come.Then after 8+ this include_type_name was removed and
type
was no longer supported.References:
https://www.elastic.co/docs/reference/elasticsearch/rest-apis/compatibility
https://www.elastic.co/guide/en/elasticsearch/reference/8.18/rest-api-compatibility.html
elastic/elasticsearch-ruby#2665
Checklist