feat(outputs): Add inlong output plugin #16211
Conversation
|
Thanks so much for the pull request! |
telegraf-tiger
bot
added
the
feat
!signed-cla |
|
@DStrand1 Hello, I have made corresponding modifications to your comments. Please take a look again. Thank you |
|
@DStrand1 Hello, I have made corresponding modifications to your comments. Please take a look again. Thank you |
There was a problem hiding this comment.
Changes look good to me, @justinwwhuang can you rebase and run go mod tidy to fix the merge issue?
justinwwhuang
force-pushed
the
b16210
branch
3 times, most recently
from
9e1915e to
30ba17d
Compare
I have made corresponding modifications to your comments. Please take a look again. Thank you |
justinwwhuang
force-pushed
the
b16210
branch
from
30ba17d to
245ff11
Compare
|
@DStrand1 I have made corresponding modifications to your comments. Please take a look again. Thank you |
|
@DStrand1 Is there anything blocking this PR? I'd appreciate feedback to improve it. |
|
|
@DStrand1 @srebhan Hello, can you help merge this PR? This PR has been in existence for a long time and has already resolved several code conflicts. If there are any issues, I am willing to make changes. |
justinwwhuang
force-pushed
the
b16210
branch
7 times, most recently
from
cc90ed1 to
de82ea4
Compare
mstrandboge
added
the
ready for final review
There was a problem hiding this comment.
Thank you very much @justinwwhuang for your contribution! Much appreciated. However, I do have some comments regarding the code. The major points are
- There is no form of user-specified option checking e.g. if no or a malformed URL is specified, missing group- or stream-IDs...
- The tests don't add any value as they will always succeed even when the plugin is set-up with invalid configuration options.
- I don't see any way to authenticate to the service. I can't imagine that anyone will use Inlong in an production environment without authentication...
While I'm okay with adding authentication later (point 3), the former ones need some more work.
For option checking (point 1) I suggest to introduce an Init() function to the plugin and check the values there with at least a test for
- provided URL is not empty
- provided URL is a valid http or https URL
- provided group-id is not empty
- provided stream-id is not empty
If the latter two are "optional" you can skip those or add a sensible default value (e.g. "telegraf").
For testing (point 2) I do see three options actually:
- Produce a mock implementation. This would require a mock of the manager "server" endpoint for getting the node IDs and a mock of the data-proxy send protocol. Information for producing the mock can be found in the inlong SDK I think together with some reverse engineering by running queries against a real instance and observe the responses. Certainly the hardest of the options but also the most rewarding one...
- Setup an integration test using docker containers by "manually" composing the instance mimicking e.g. this docker compose setup. This would guarantee that the plugin would actually work with a real Inlong instance...
- If none of the above is possible, at least add some tests for invalid user options and skip all the always-succeeding mock-producer tests.
My preference would be a mix of 1 and 2 but that might be some effort.
So long story short, I would need at least the user-setting checks and a rudimentary test being implemented to be able to merge this PR.
| func TestInlong_Write(t *testing.T) { | ||
| s := &csv.Serializer{Header: true} | ||
| require.NoError(t, s.Init()) | ||
| producer := &MockProducer{} | ||
| i := &Inlong{ | ||
| producer: producer, | ||
| serializer: s, | ||
| } | ||
| m := metric.New( | ||
| "cpu", | ||
| map[string]string{ | ||
| "topic": "test-topic", | ||
| }, | ||
| map[string]interface{}{ | ||
| "value": 42.0, | ||
| }, | ||
| time.Unix(0, 0), | ||
| ) | ||
| var metrics []telegraf.Metric | ||
| metrics = append(metrics, m) | ||
| require.NoError(t, i.Write(metrics)) | ||
| } |
There was a problem hiding this comment.
To be honest, I don't know if this test adds any meaningful guard at all. The mock producer will not validate any of the data sent nor any of the parameters. It would succeed even if you don't specify any URL or group or stream (as you do here) and without a connect! So you could do anything and this test would always succeed! That's not a test in my view.
| ManagerURL string `toml:"manager_url"` | ||
| Log telegraf.Logger `toml:"-"` | ||
|
|
||
| producerFunc func(groupId string, managerUrl string) (dataproxy.Client, error) |
There was a problem hiding this comment.
This indirection through producerFunc seems to be introduced for testing. However, as it stands now, the tests add no real value so I would remove the indirection in favor of simplifying the code.
| return nil | ||
| } | ||
|
|
||
| func (i *Inlong) Write(metrics []telegraf.Metric) error { |
There was a problem hiding this comment.
What happens if the Inlong instance goes down for whatever reason and comes up again e.g. due to network issues or service outages? Would the plugin reconnect automatically in this case?
srebhan
removed
the
ready for final review
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
…igs ahead of time
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
@justinwwhuang please re-request a review once you are done, for me to get a notification and guarantee fast update-review cycles... You are already waiting for too long... :-( |
Summary
Add inlong output plugin
Apache InLong is a one-stop, full-scenario integration framework for massive data that supports Data Ingestion, Data Synchronization and Data Subscription, and it provides automatic, secure and reliable data transmission capabilities. InLong also supports both batch and stream data processing at the same time, which offers great power to build data analysis, modeling and other real-time applications based on streaming data.
Checklist
Related issues
resolves #16210