Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(outputs.quix): Add plugin #16144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tomas-quix
Copy link

@tomas-quix tomas-quix commented Nov 5, 2024

First implementation - no defaults

Summary

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #

!signed-cla

First implementation - no defaults
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Nov 5, 2024

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@srebhan srebhan changed the title Ouix output plugin feat(outputs.quix): Add plugin Nov 12, 2024
@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Nov 12, 2024
@srebhan srebhan self-assigned this Nov 12, 2024
@tomas-quix
Copy link
Author

tomas-quix commented Nov 19, 2024

!signed-cla

1 similar comment
@stereosky
Copy link

!signed-cla

@tomas-quix tomas-quix marked this pull request as ready for review November 19, 2024 10:30
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution @tomas-quix! I do have some comments in the code. Furthermore, I want to encourage you to implement some unit-tests for this plugin, e.g. by mocking the config server and using a quix docker container or at least using a kafka instance mocking the replies...

Comment on lines +12 to +14
This plugin uses an SDK token for authentication with Quix. You can generate
one in the settings under the `API and tokens` section by clicking on
`SDK Token`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This plugin uses an SDK token for authentication with Quix. You can generate
one in the settings under the `API and tokens` section by clicking on
`SDK Token`.
This plugin uses a SDK token for authentication with Quix. You can generate
one in the settings under the `API and tokens` section by clicking on
`SDK Token`.

Do you also have a link here to some documentation section?

Comment on lines +3 to +8
This plugin writes metrics to a [Quix](https://quix.io/) endpoint.

Please consult Quix's [official documentation](https://quix.io/docs/) for more
details on the Quix platform architecture and concepts.

⭐ Telegraf v1.32.2 🏷️ cloud, messaging 💻 all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This plugin writes metrics to a [Quix](https://quix.io/) endpoint.
Please consult Quix's [official documentation](https://quix.io/docs/) for more
details on the Quix platform architecture and concepts.
⭐ Telegraf v1.32.2 🏷️ cloud, messaging 💻 all
This plugin writes metrics to a [Quix](https://quix.io/) endpoint.
Please consult Quix's [official documentation][quick] for more
details on the Quix platform architecture and concepts.
⭐ Telegraf v1.33.0 🏷️ cloud, messaging 💻 all
[quix]: https://quix.io/docs/

Comment on lines +37 to +42
For this output plugin to function correctly the following variables must be
configured.

* workspace
* auth_token
* topic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments on the sample.conf file!

Comment on lines +64 to +70
### data_format

The data format for serializing the messages. Defaults to `json`.

### timestamp_units

The timestamp units for precision. Defaults to `1s` for one second.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you support any other setting with this version of the output? If not, please remove the options.

Comment on lines +1 to +7
[[outputs.quix]]
workspace = "your_workspace"
auth_token = "your_auth_token"
topic = "telegraf_metrics"
api_url = "https://portal-api.platform.quix.io"
data_format = "json"
timestamp_units = "1s"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note, the first line of the config must be a comment containing a brief description of the plugin. All options must be documented starting with a double-hash! Default values must be commented with the value being the default using a single-hash.

Furthermore, you should shorten the option names without sacrificing the meaning...

Like

Suggested change
[[outputs.quix]]
workspace = "your_workspace"
auth_token = "your_auth_token"
topic = "telegraf_metrics"
api_url = "https://portal-api.platform.quix.io"
data_format = "json"
timestamp_units = "1s"
# Sending metrics to a Quix data processing pipeline
[[outputs.quix]]
## Endpoint accepting the data
# url = "https://portal-api.platform.quix.io"
## Workspace and topics to send the metrics to
workspace = "your_workspace"
topic = "telegraf_metrics"
## Authentication token created in Quix
token = "your_auth_token"

q.Log.Errorf("Error sending message to Kafka: %v", err)
}
}
q.Log.Debugf("Metrics sent to Quix.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
q.Log.Debugf("Metrics sent to Quix.")

// Close shuts down the Kafka producer
func (q *Quix) Close() error {
if q.producer != nil {
q.Log.Infof("Closing Quix producer connection.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
q.Log.Infof("Closing Quix producer connection.")

)

// XDGSCRAMClient wraps the SCRAM client for SCRAM-SHA-256 authentication
type XDGSCRAMClient struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to export this struct?

Comment on lines +36 to +38
// Define SHA256 and SHA512 hash generators
var SHA256 scram.HashGeneratorFcn = scram.SHA256
var SHA512 scram.HashGeneratorFcn = scram.SHA512
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? It's not used anywhere...

Comment on lines +1 to +38
// scram_client.go
package quix

import (
"github.com/xdg-go/scram"
)

// XDGSCRAMClient wraps the SCRAM client for SCRAM-SHA-256 authentication
type XDGSCRAMClient struct {
*scram.Client
*scram.ClientConversation
HashGeneratorFcn scram.HashGeneratorFcn
}

// Begin initializes the SCRAM client with username and password
func (x *XDGSCRAMClient) Begin(userName, password, authzID string) error {
client, err := x.HashGeneratorFcn.NewClient(userName, password, authzID)
if err != nil {
return err
}
x.Client = client
x.ClientConversation = client.NewConversation()
return nil
}

// Step processes the server's challenge and returns the client's response
func (x *XDGSCRAMClient) Step(challenge string) (string, error) {
return x.ClientConversation.Step(challenge)
}

// Done returns true if the SCRAM conversation is complete
func (x *XDGSCRAMClient) Done() bool {
return x.ClientConversation.Done()
}

// Define SHA256 and SHA512 hash generators
var SHA256 scram.HashGeneratorFcn = scram.SHA256
var SHA512 scram.HashGeneratorFcn = scram.SHA512
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file is a blunt copy of plugins/common/kafka/scram_client.go. Why do we need this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants