[chore][exporter/elasticsearch] refactor, consolidate exporters #33318
Conversation
Replace the elasticsearchTracesExporter and elasticsearchLogsExporter with a single elasticsearchExporter that handles both. Tests have been cleaned up to reduce repetition between traces and logs exporters, e.g. config validation tests have been moved to more specific TestConfig* tests. To enable this, some further validation has been added to Config.Validate, which was previousy done only when reaching the go-elasticsearch code when constructing exporters. Various tests have been updated to use the public APIs rather than internal functions, so we're testing behaviour rather than the implementation; this will enable further refactoring without breaking tests.
axw
changed the title
| @@ -28,7 +27,7 @@ type elasticsearchTracesExporter struct { | |||
| model mappingModel | |||
| } | |||
|
|
|||
| func newTracesExporter(logger *zap.Logger, cfg *Config) (*elasticsearchTracesExporter, error) { | |||
| func newExporter(logger *zap.Logger, cfg *Config, index string, dynamicIndex bool) (*elasticsearchExporter, error) { | |||
There was a problem hiding this comment.
[Non blocking/discussion] I was thinking of refactoring this a bit more (possibly in the future) to use the same instance of the exporter (and thus the bulk indexer) for both logs and traces (and eventually metrics) - IMO, this will allow us to better reason about resource consumption.
There was a problem hiding this comment.
I like the idea, but I'm unsure how one would go about it. Is there any prior art? Probably worth opening an issue so this idea and any discussion doesn't get lost.
There was a problem hiding this comment.
There is SharedComponent however we can't probably use it as is.
Probably worth opening an issue so this idea and any discussion doesn't get lost.
Sounds good, I will create an issue.
There was a problem hiding this comment.
Created an issue: #33326
I have a few ideas on how this could look like. Will create a followup PR for discussions after this PR is merged.
Co-authored-by: Carson Ip <carsonip@users.noreply.github.com>
| @@ -28,7 +27,6 @@ elasticsearch/log: | |||
| endpoints: [http://localhost:9200] | |||
| logs_index: my_log_index | |||
| timeout: 2m | |||
| cloudid: TRNMxjXlNJEt | |||
There was a problem hiding this comment.
Looks no breaking changes this PR, except here why delete cloudid? others LGTM
There was a problem hiding this comment.
I had to remove it because of the changes to Config.Validate:
- this
cloudidvalue is invalid - it's not permitted to have both
cloudidandendpointsset
These would already have caused failures before, but only when creating the exporter, I just moved it to Config.Validate so the error occurs sooner.
There's a new valid entry called elasticsearch/cloudid which covers it.
Description:
Replace the
elasticsearchTracesExporterandelasticsearchLogsExporterwith a singleelasticsearchExporterthat handles both.Tests have been cleaned up to reduce repetition between traces and logs exporters, e.g. config validation tests have been moved to more specific
TestConfig*tests. To enable this, some further validation has been added toConfig.Validate, which was previously done only when reaching the go-elasticsearch code when constructing exporters.Various tests have been updated to use the public APIs rather than internal functions, so we're testing behaviour rather than the implementation; this will enable further refactoring without breaking tests.
Link to tracking Issue:
None
Testing:
Ran the unit tests. This is a non-functional change.
Documentation:
N/A, non-functional change.