feat: add include_source_on_error param to bulk index requests #229
Conversation
elastic-observability-automation
bot
added
the
safe-to-test
The elastictransport doesn't include the ES api so it's not tied to an ES/api version. |
There was a problem hiding this comment.
It might present a problem with backward compatibility, as this code change introduces version dependency on ES, which may not be feasible for elasticsearchexporter.
A path forward may be to make this behavior configurable, to allow es exporter to opt out of this and use the manual sanaitization code path.
bulk_indexer.go
Outdated
| v := req.URL.Query() | ||
| if b.config.Pipeline != "" { | ||
| v.Set("pipeline", b.config.Pipeline) | ||
| } | ||
| if b.config.RequireDataStream { | ||
| v.Set("require_data_stream", strconv.FormatBool(b.config.RequireDataStream)) | ||
| } | ||
| v.Set("filter_path", "items.*._index,items.*.status,items.*.failure_store,items.*.error.type,items.*.error.reason") | ||
| v.Set("filter_path", strings.Join([]string{"items.*._index", "items.*.status", "items.*.failure_store", "items.*.error.type", "items.*.error.reason"}, ",")) | ||
| v.Set("include_source_on_error", "false") |
There was a problem hiding this comment.
Q: what happens when this is passed to a ES <8.18? If it will fail the request, doesn't it mean that elasticsearch exporter cannot bump docappender, as es exporter doesn't have strict dependency on ES version?
There was a problem hiding this comment.
it gives HTTP 400
POST /_bulk?include_source_on_error=true
{"create":{"_index":"logs-foo-bar"}}
{"@timestamp":"1", "foo":"bar"}
{
"error": {
"root_cause": [
{
"type": "illegal_argument_exception",
"reason": "request [/_bulk] contains unrecognized parameter: [include_source_on_error]"
}
],
"type": "illegal_argument_exception",
"reason": "request [/_bulk] contains unrecognized parameter: [include_source_on_error]"
},
"status": 400
}
There was a problem hiding this comment.
I agree with Carson on this, there is a wider question about backward compatibility guarantees that we provide in this library. Since we version it independently from the stack version, do we need to always preserve backward compatibility with ES? could we break it with a major version bump?
There was a problem hiding this comment.
A path forward may be to make this behavior configurable, to allow es exporter to opt out of this and use the manual sanaitization code path.
You are raising an important point here @carsonip . While we could just bump to a major version after adding the change, it would make it much harder to use it and upgrade to the latest version downstream.
Making it configurable sounds like the right direction.
There was a problem hiding this comment.
@kruskall has already agreed on making this configurable in another discussion. The remaining bit will be whether to
- keep the old sanitization logic in docappender; or
- remove that logic and offload that to users (e.g. es exporter) requiring extra ES compatibility
There was a problem hiding this comment.
enum sgtm. If we're exposing these under docappender package, users will be calling it docappender.True which is a bit strange. What about IncludeSourceTrue, IncludeSourceFalse, IncludeSourceTrueSanitize?
There was a problem hiding this comment.
Will the introduction of include_source_on_error conflict with backward compatibility regardless? In #229 (comment) @carsonip confirmed that older ES version will reject a request with include_source_on_error option. So even if we go with 3 state configuration option for include_source_on_error either with *bool or enum, users will still have to know somehow to not use anything but the default value of this option when they target older ES.
There was a problem hiding this comment.
As stated in the config comment, both True and False requires 8.18+. It'll be fine in apm server, if apm server <8.18 doesn't bump docappender. In es exporter we'll stick with Unset for a while. What kind of backward compatibility are you referring to here?
// Requires Elasticsearch 8.18+ if value is True or False.
There was a problem hiding this comment.
The worry here is that for an external library user it might be hard to discover this note, so in a hypothetical situation when someone updates this library and sets the flag with a good intention they will end up with a broken client.
I understand that setting safe default make it safe from backward compatibility perspective, but it kind of makes it more easy to misuse and claim that the library is not backward compatibility.
But regardless it's a minor concern and more a hypothetical situation, so I don't have any strong objection against either of options that were discussed.
There was a problem hiding this comment.
Thank you for raising the concern!
The backward compatibility guarantee is only for default options. I see your point but IMO we found the right balance by adding an option that require a more recent version of ES (along with proper documentation). Happy to discuss this more if you want 🙂
Co-authored-by: Carson Ip <[email protected]>
Co-authored-by: Carson Ip <[email protected]>
| @@ -843,9 +846,9 @@ func TestAppenderIndexFailedLogging(t *testing.T) { | |||
| require.Len(t, entries, N/2) | |||
| assert.Equal(t, "failed to index documents in 'an_index' (document_parsing_exception): ", entries[0].Message) | |||
| assert.Equal(t, int64(2), entries[0].Context[0].Integer) | |||
| assert.Equal(t, "failed to index documents in 'an_index' (error_type): error_reason_even", entries[1].Message) | |||
| assert.Equal(t, "failed to index documents in 'an_index' (error_type): ", entries[1].Message) | |||
Co-authored-by: Carson Ip <[email protected]>
❓ Why is this being changed
At the moment ES might return the source a request in the error response. Set the parameter to avoid the fragile parsing logic and make bulk request handling more robust.
🧑💻 What is being changed
Set
IncludeSourceOnErrortofalse✅ How to validate the change
unfortunately this can only be validate manually since it relies on an ES feature