Fix Elasticsearch version in ES OTEL writer #2409
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
| if err != nil { | ||
| return nil, err | ||
| } | ||
| logger.Info("Elasticsearch detected", zap.Int("version", esVersion)) | ||
| params.Version = uint(esVersion) |
There was a problem hiding this comment.
The ES version is derived here in the client factory method and assigned to the config. However the config is passed by value, hence the changes are not reflected after this function. Then the wrong value of the version was used by the called to create index template.
Instead of overriding the config it is cleaner to expose the version on the client.
Signed-off-by: Pavol Loffay <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2409 +/- ##
==========================================
- Coverage 95.56% 95.54% -0.03%
==========================================
Files 208 208
Lines 10676 10676
==========================================
- Hits 10203 10200 -3
- Misses 401 403 +2
- Partials 72 73 +1
Continue to review full report at Codecov.
|
| esPing := elasticsearchPing{ | ||
| username: params.Username, | ||
| password: params.Password, | ||
| roundTripper: roundTripper, | ||
| } | ||
| esVersion, err := esPing.getVersion(params.Servers[0]) | ||
| esVersion, err = esPing.getVersion(params.Servers[0]) |
There was a problem hiding this comment.
Does it make sense to check length of .Servers before dereferencing? It does appear that right now it's impossible to pass Servers with length 0 into this method, but the code that prevents that is fairly distant from this.
Might be better if this method protects itself.
There was a problem hiding this comment.
+1 I am will submit a follow-up PR to fix that
|
I have defined the --es.version=7, but in es v7.15.2 already not working. |
Signed-off-by: Pavol Loffay [email protected]
Resolves #2407