Add Cassandra OTEL exporter #2139
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2139 +/- ##
==========================================
- Coverage 96.15% 96.14% -0.02%
==========================================
Files 218 218
Lines 10567 10572 +5
==========================================
+ Hits 10161 10164 +3
Misses 352 352
- Partials 54 56 +2
Continue to review full report at Codecov.
|
pavolloffay
changed the title
pavolloffay
requested review from
objectiser
and removed request for
jpkrohling
Signed-off-by: Pavol Loffay <[email protected]>
|
|
||
| cassandraFactory := factories.Exporters[cassandra.TypeStr] | ||
| cc := cassandraFactory.CreateDefaultConfig().(*cassandra.Config) | ||
| assert.Equal(t, []string{"127.0.0.1"}, cc.Options.GetPrimary().Servers) |
There was a problem hiding this comment.
No harm in testing again, but default values are tested in cmd/opentelemetry-collector/app/exporter/cassandra/config_test.go
There was a problem hiding this comment.
Double-check that the default values have been applied. I will consider changing defaults interface to include cobra command and install Jaeger flags by default.
| Index IndexConfig `mapstructure:"index"` | ||
| } | ||
|
|
||
| // IndexConfig configures indexing. |
|
|
||
| // Factory is the factory for Jaeger Cassandra exporter. | ||
| type Factory struct { | ||
| OptionsFactory OptionsFactory |
There was a problem hiding this comment.
in what scenarios does this need to be a function instead of a struct?
There was a problem hiding this comment.
We need to initialize the Options struct with wiper once the collector service has started.
We can revisit this and instead pass a viper instance and run the initialization in CreateDefaultConfig.
| } | ||
|
|
||
| // CreateDefaultConfig returns default configuration of Factory. | ||
| func (f Factory) CreateDefaultConfig() configmodels.Exporter { |
There was a problem hiding this comment.
Is the name CreateDefaultConfig required by some external API? We usually mention it in the comment.
Given that the result of this function depends on externally provided (via OptionsFactory) options, the name "default" is confusing.
There was a problem hiding this comment.
CreateDefaultConfig() is part of the extension.Factory interface in the Otel collector. I agree that "default" is confusing here, we should ideally apply user supplied options once the default config is created
There was a problem hiding this comment.
I agree that "default" is confusing here, we should ideally apply user supplied options once the default config is created
OTEL service applies config file changes to the returned instance. We cannot override them by config supplied as legacy jaeger flags. The OTEL config takes precedence over legacy jaeger configuration.
| import ( | ||
| "github.com/open-telemetry/opentelemetry-collector/config/configmodels" | ||
|
|
||
| "github.com/jaegertracing/jaeger/plugin/storage/cassandra" |
There was a problem hiding this comment.
Also we should consider naming this import storageCassandra. Less confusing and also consistent with other files where it has been imported
There was a problem hiding this comment.
It is only in one file, where there is a conflict in imports.
Signed-off-by: Pavol Loffay <[email protected]>
|
@yurishkuro I will merge this, feel free to comment and I will fix issues in follow up PR. |
Depdens on #2135
Signed-off-by: Pavol Loffay [email protected]