Add SAMPLING_STORAGE_TYPE environment variable #3573
Conversation
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #3573 +/- ##
==========================================
+ Coverage 96.46% 96.51% +0.04%
==========================================
Files 264 264
Lines 15429 15447 +18
==========================================
+ Hits 14884 14909 +25
+ Misses 461 453 -8
- Partials 84 85 +1
Continue to review full report at Codecov.
|
|
Just an observation - this will be subject to the existing limitation that a given kind of storage (e.g. cassandra) can only be configured once, i.e. it's not possible to have span and sampling storage use different instances of cassandra (or even different namespaces). I don't think it should block this PR, but it may be worth thinking how that can be improved. For example, we have the mechanism for configuring different "named" storage, namely "archive", via flags like Of course, if the new solution prevents sharing the same instance as we can today that would be a regression. |
There was a problem hiding this comment.
LGTM. Does this mean we need to add support for SAMPLING_STORAGE_TYPE in jaeger-operator similar to jaegertracing/jaeger-operator#1700?
| @@ -37,15 +38,18 @@ func TestFactoryConfigFromEnv(t *testing.T) { | |||
| assert.Equal(t, cassandraStorageType, f.SpanWriterTypes[0]) | |||
| assert.Equal(t, cassandraStorageType, f.SpanReaderType) | |||
| assert.Equal(t, cassandraStorageType, f.DependenciesStorageType) | |||
| assert.Equal(t, "", f.SamplingStorageType) | |||
There was a problem hiding this comment.
nit: could use the more declarative assert.Empty(t, f.SamplingStorageType)
Oof, I don't know much about the operator. I had intended to PR the docs, but can't say I feel confident in my ability to make a well thought out operator change.
This is a good point. I will include a note about this in my docs PR. |
|
I'm unable to merge b/c of the Codecov failure. Can you merge? or should i try to catch the last line or so :)? |
* Added SAMPLING_STORAGE_TYPE support in factory config Signed-off-by: Joe Elliott <number101010@gmail.com> * Added factory support and tests Signed-off-by: Joe Elliott <number101010@gmail.com> * review Signed-off-by: Joe Elliott <number101010@gmail.com> Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>
Which problem is this PR solving?
SAMPLING_STORAGE_TYPE. This allows the user to independently configuretheir
SPAN_STORAGE_TYPEandSAMPLING_STORAGE_TYPE. e.g. the user could choose to store their spans in elasticsearch while putting their adaptive sampling data in cassandra. This also paves the way for simpler to operate sampling storage types like redis or memcached.Short description of the changes
SAMPLING_STORAGE_TYPEto plugin/storage/factory_config.go