Add Elasticsearch OTEL exporter #2140
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2140 +/- ##
==========================================
- Coverage 96.14% 96.12% -0.02%
==========================================
Files 218 218
Lines 10572 10577 +5
==========================================
+ Hits 10164 10167 +3
- Misses 352 354 +2
Partials 56 56
Continue to review full report at Codecov.
|
|
@annanay25 Would you be able to review? |
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
| func New(config *Config, log *zap.Logger) (exporter.TraceExporter, error) { | ||
| factory := es.NewFactory() | ||
| factory.InitFromOptions(config.Options) | ||
| err := factory.Initialize(metrics.NullFactory, log) |
There was a problem hiding this comment.
factory has two init functions, possible to merge?
There was a problem hiding this comment.
There is a difference between them, the first one just initializes the props and the second one creates a connection to the storage.
|
|
||
| // DefaultOptions creates Elasticsearch options supported by this exporter. | ||
| func DefaultOptions() *es.Options { | ||
| return es.NewOptions("es") |
There was a problem hiding this comment.
Probably a more suitable namespace is "jaeger" / "jaeger-otel" since the es cluster might be used by multiple services
There was a problem hiding this comment.
It has to be es because we want to be backward compatible with the configuration with jaeger collector. This name is used in flag names e.g. --es.server-urls=
| @@ -59,7 +61,7 @@ const ( | |||
| // to bind them to command line flag and apply overlays, so that some configurations | |||
| // (e.g. archive) may be underspecified and infer the rest of its parameters from primary. | |||
| type Options struct { | |||
| primary *namespaceConfig | |||
| Primary namespaceConfig `mapstructure:",squash"` | |||
There was a problem hiding this comment.
Why are we removing pointer reference here?
There was a problem hiding this comment.
The mapstructure does not work with pointers. It also has to be made public.
| @@ -233,7 +236,7 @@ func initFromViper(cfg *namespaceConfig, v *viper.Viper) { | |||
| cfg.Password = v.GetString(cfg.namespace + suffixPassword) | |||
| cfg.TokenFilePath = v.GetString(cfg.namespace + suffixTokenPath) | |||
| cfg.Sniffer = v.GetBool(cfg.namespace + suffixSniffer) | |||
| cfg.servers = stripWhiteSpace(v.GetString(cfg.namespace + suffixServerURLs)) | |||
| cfg.Servers = strings.Split(stripWhiteSpace(v.GetString(cfg.namespace+suffixServerURLs)), ",") | |||
There was a problem hiding this comment.
IIRC there's an easier way to do this with viper, using GetStringSlice.
There was a problem hiding this comment.
I could not make it work with the format of expected values url, url2
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay [email protected]
#2139 should go in first and this should reuse storage factory helper.