InfluxDB Connector for GoFr #2072
Conversation
…operations, [AddInfluxDB()] method for externalized datasource added. [logging] standard app logging implemented for influxdb
…y organisation name.
…zation, Ping, ListBuckets, DeleteBucket, all got implemented.
coolwednesday
changed the title
There was a problem hiding this comment.
- Create a separate
go.modforpkg/gofr.datasource/influxdband please make sure no unnecessary imports are added to GoFr'sgo.mod. - No test files added. Please make sure to try and achieve atleast 80-85% code coverage in the newly added directory
influxdb. - GoDoc comments are not proper and lacks info about the method.
- Your newly added code has a lot of linter errors in
pkg/gofr/datasource/influxdb. Please address and resolve them. - The file
interfaces.gois empty. If you don;t need it you can delete it but i think you need it for testing and other purposes. Please refer to other datasources and you will understand. - Let's add screenshots for InfluxDB logs and queries to see how they look after pretty printed.
|
@Umang01-hash thanks for the review, i am working on it. |
|
@Umang01-hash please run the CI, i have written the test and fixed the linting issue, let's see, also achieved the the 72% coverage in influxdb. also I haven't mocked the influxdb, maybe that's why coverage is low, please let me know if i'ts required to mock that. (actually, i'm new to mocking), also review the PR. |
|
please rerun @Umang01-hash |
There was a problem hiding this comment.
@akhilsharmaa, After doing the review changes. Please add a screenshot of the running server with DB Calls with proper logs, traces and metrics.
Please also add the docker command to setup an db instance in the contributing.md as well as docs for developer's ease of setup for exploring and testing as well.
Thank You !
Well Done : )
There was a problem hiding this comment.
So we also have a mock container. I would need you to generate InFluxDB mocks and add it there.
There was a problem hiding this comment.
Do refer to SendOperationStats method in other datasources that adds traces and metrics in the respective datasources' methods. We would need you to also add them.
| func (c *Client) UseLogger(logger any) { | ||
| if l, ok := logger.(Logger); ok { | ||
| c.logger = l | ||
| } | ||
| } | ||
|
|
||
| func (c *Client) UseMetrics(metrics any) { | ||
| if m, ok := metrics.(Metrics); ok { | ||
| c.metrics = m | ||
| } | ||
| } |
There was a problem hiding this comment.
It should not be any as the parameter. Refer to mongo datasource for method signature and modify this accordingly.
Thanks, i am working on the changes. |
docs/datasources/influxdb/page.md
Outdated
| fmt.Println("Ping failed:", err) | ||
| return | ||
| } | ||
| fmt.Println("InfluxDB connected:", ok) |
| UseLogger(logger any) | ||
| UseMetrics(metrics any) | ||
| UseTracer(tracer any) |
There was a problem hiding this comment.
This is not exposed to user. Refer to mongoDB datasource addition for more info.
docs/datasources/influxdb/page.md
Outdated
| return | ||
| } | ||
| fmt.Println("Organization deleted successfully") | ||
|
|
There was a problem hiding this comment.
Write a setup function .... pseudo and then make REST endpoints for adding, retrieving data.
|
@akhilsharmaa , Logging , metrics, traces have not been added. Refer to mongo datasource package or this PR : #2076, for reference. |
… not implemented.
|
@akhilsharmaa There are quite a few merge conflicts in your PR. I strongly recommend pulling the latest changes from the development branch regularly and resolving conflicts as they arise. Delaying this may lead to more complex and extensive code changes later on. |
@Umang01-hash thanks, i have resolved the conflicts, waiting for the review. :) |
There was a problem hiding this comment.
Hey @akhilsharmaa here are a few key changes for your PR:
- The current doucmentation inside
docs/datasources/influxdbis nice but we try to maintain consistency with other datasource documentations. Please refer to any other datasource doucmentation (ex: mongo/cassandra). You currently have extra methods and the starting should also be improved. - The file
mock_container.gohas major errors and unintentional changes. Please resolve them (Unresolved reference 'dgraphMock',Unresolved reference 'opentsdbMock'etc.) These changes were also not related to your PR so please revert them back and add only influxdb mock where needed in mock container. - The file
mock_interface.goshould be placed outside of themocksdirectory insidedatasource/influxdb. No need to createmocksdirectory inside influxdb. - As far as i can see the influxdb has currently only logs but no metrics and traces. You can refer to other datasources to see how are these added. (Look at method sendOperationStats)
- All the methods that you have listed inside
datasource/influxdb/interface.go(27 methods) are not same as the one you declared insidecontainer/datasources.go(9 methods). You need to figure it out and try to place a nearly similar interface there too to maintain consistency. - Also i am not getting the use of
influxdb/internal.go. Can you please put some light on it. - You should also add test for
AddInfluxDBadded inexternal_db.go. - Currently your code quality pipeline is failing and it may be due to errors in
mock_container.goso once you fix that please recheck on any linter errors.
There was a problem hiding this comment.
@Umang01-hash , This internal is just wrappers, main purpose was to clean implementation of mocking. (instead of generating all the functions of deafault interface, just wrapper it).
WIP
i have resolved, please confirm it once.
yes, i have moved it now.
yes, will implement it soon.
Actually
This internal is just wrappers, main purpose was to clean implementation of mocking. (instead of generating all the functions of deafault interface, just wrapper it).
Fixed :) |
|
@Umang01-hash please run the workflow, seems pipline is fixed. |
InfluxDB Connector for GoFr
Closes: #1994
Contributor: Akhilesh Kumar Sharma
Track: Core Framework
Overview
This PR introduces native support for InfluxDB, a widely-used time-series database, to GoFr. With this integration, GoFr can now seamlessly store and query high-frequency data such as metrics and sensor data, enhancing its capabilities for time-series workloads.
Problem
Currently, GoFr does not support InfluxDB, which restricts its ability to effectively manage time-series data. This integration addresses that limitation by adding the necessary functionality.
Scope
The following changes and features have been implemented in this PR:
pkg/gofr/datasource/influxdb.Appto utilize the datasource.Checklist
goimportandgolangci-lint.