[jaeger-v2] Add support for Cassandra #5253
Conversation
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
|
@james-ryans @akagami-harsh, please review. Currently, TestCassandraStorageFactoryWithConfig is failing with a timeout error. Maybe some config issue??? |
It is because you are trying to connect to HTTP server with Cassandra client, if you set the config with That is because Cassandra connection is over TCP instead of HTTP. Perhaps, you should try to spawn a TCP server instead of HTTP server, here I found a guide to create one but I haven't successfully implement it for Cassandra connection. https://okanexe.medium.com/the-complete-guide-to-tcp-ip-connections-in-golang-1216dae27b5a |
Signed-off-by: Pushkar Mishra <[email protected]>
|
@yurishkuro, I cannot create a connection between the test and Cassandra. Everytime, i try something, I get this error : Received unexpected error: gocql: unable to create session: unable to discover protocol versionDo you have any idea how I can do it? In other tests, the first factory is created with f.NewFactory(), the mock session is passed in, and then it checks noError in f.initialize. But, here, we can't do the same. since both f.NewFactory() and f.Initialize() is part of the same function NewFactoryWithConfig(). f := NewFactory()
f.primaryConfig = newMockSessionBuilder(session, nil)
require.NoError(t, f.Initialize(metrics.NullFactory, logger))edit : Adding on slack also, to get more ideas or suggestions. |
|
now do we do that in Elastic? It also uses NewFactoryWithConfig pattern. |
|
Yes, in ElasticSearch we use the same NewFactoryWithConfig pattern. But there, in the test, we created a mock HTTP server to test, but it is not working here. -> Timeout Error var mockCassandraResponse = []byte(`
{
"Version": "3.11.11"
}
`)
func TestCccFactoryWithConfig(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write(mockCassandraResponse)
}))
defer server.Close()
serverURL := server.URL
serverURL = serverURL[len("http://"):]
link, portStr, err := net.SplitHostPort(serverURL)
require.NoError(t, err)
port, err := strconv.Atoi(portStr)
require.NoError(t, err)
cfg := cassandraCfg.Configuration{
Servers: []string{link},
Keyspace: "test",
ConnectTimeout: time.Second * 5,
Port: port,
}
factory, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop())
require.NoError(t, err)
defer factory.Close()
} Error Trace: /home/pushkar/jaeger/plugin/storage/cassandra/factory_test.go:235
Error: Received unexpected error:
gocql: unable to create session: unable to discover protocol version: gocql: no response received from cassandra within timeout period
Test: TestCccFactoryWithConfig
--- FAIL: TestCccFactoryWithConfig (5.02s) |
|
Creating a custom server is a useful pattern. Can we do the same for Cassandra? It may be more difficult as I don't think it's using HTTP protocol. |
|
Yeah, we can create a custom server for Cassandra. Can you provide some hints/examples? EDIT : https://github.com/jaegertracing/jaeger/pull/4971/files : this seems relevant |
|
Well again, you can see the example in ES tests. But ES uses HTTP so it's much easier to mock. I don't know if Cassandra can use HTTP protocol. If it's only some bespoke binary protocol, than it's harder to mock (perhaps there are libraries out there for that, or perhaps gocql has a place were we can substitute the actual server communication). There are some tricks we can do internally by not combining creation and connection in a single step. |
Signed-off-by: Pushkar Mishra <[email protected]>
|
removing unit tests |
Signed-off-by: Pushkar Mishra <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5253 +/- ##
==========================================
- Coverage 95.11% 95.05% -0.07%
==========================================
Files 339 339
Lines 16511 16529 +18
==========================================
+ Hits 15705 15712 +7
- Misses 618 629 +11
Partials 188 188
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Pushkar, I believe what Yuri meant previously was that we should only leave the unit test that needs to connect to Cassandra (which we can't mock) and keep all other unit tests you successfully created. What do you think? |
|
ok, we can do it @james-ryans. btw i created a weekly milestone doc, do check it out. |
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
yurishkuro
added
v2
changelog:exprimental
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
pkg/cassandra/config/config.go
Outdated
| @@ -29,21 +30,21 @@ import ( | |||
|
|
|||
| // Configuration describes the configuration properties needed to connect to a Cassandra cluster | |||
| type Configuration struct { | |||
| Servers []string `validate:"nonzero" mapstructure:"servers"` | |||
| Servers []string `validate:"nonzero" mapstructure:"servers" valid:"required,url"` | |||
There was a problem hiding this comment.
do we know what would be using the validate: tags? Maybe we should convert them to valid:?
There was a problem hiding this comment.
go-playground/validator is using the validate: tags. Since we are using asaskevich/govalidator, we should change this to valid:.
|
working on it |
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test