feat(outputs.azure_data_explorer): Added MS_Fabric with refactoring #16372
Conversation
* Refactored ADX and EH plugin to reuse common code. * Added corresponding test cases
telegraf-tiger
bot
added
feat
* Minor changes
…om/asaharn/telegraf into feature/microsoft_fabric_connector
asaharn
force-pushed
the
feature/microsoft_fabric_connector
branch
from
597ed1f to
4e7821a
Compare
…om/asaharn/telegraf into feature/microsoft_fabric_connector
|
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 |
There was a problem hiding this comment.
@asaharn thanks for your contribution. I do have some comments in the code. Additionally, as a general comment, I suggest to split this PR into the following individual PRs:
- Move the
azure_data_explorercommon part toplugins/common/adxand convert theazure_data_explorerplugin to use it. - Move the
event_hubscommon part toplugins/common/event_hubsand convert theevent_hubsplugin to use it. - Implement the new
microsoft_fabricplugin based on the common parts.
I can assist with step 1 to give you a climpse on how I think things should look like... Let me know if you want me to help!
| @@ -0,0 +1,286 @@ | |||
| package adx_commons | |||
There was a problem hiding this comment.
Please name the package adx as the directory.
| "github.com/Azure/azure-kusto-go/kusto" | ||
| kustoerrors "github.com/Azure/azure-kusto-go/kusto/data/errors" | ||
|
|
||
| "github.com/Azure/azure-kusto-go/kusto/ingest" | ||
| "github.com/Azure/azure-kusto-go/kusto/kql" |
There was a problem hiding this comment.
| "github.com/Azure/azure-kusto-go/kusto" | |
| kustoerrors "github.com/Azure/azure-kusto-go/kusto/data/errors" | |
| "github.com/Azure/azure-kusto-go/kusto/ingest" | |
| "github.com/Azure/azure-kusto-go/kusto/kql" | |
| "github.com/Azure/azure-kusto-go/kusto" | |
| kustoerrors "github.com/Azure/azure-kusto-go/kusto/data/errors" | |
| "github.com/Azure/azure-kusto-go/kusto/ingest" | |
| "github.com/Azure/azure-kusto-go/kusto/kql" |
| "github.com/influxdata/telegraf/plugins/serializers/json" | ||
| ) | ||
|
|
||
| var sampleConfig string |
There was a problem hiding this comment.
This is not something that should belong in common, this is plugin specific!
| type AzureDataExplorer struct { | ||
| Endpoint string `toml:"endpoint_url"` | ||
| Database string `toml:"database"` | ||
| Log telegraf.Logger `toml:"-"` |
There was a problem hiding this comment.
Please move the logger to the plugin(s)!
| tableMetricGroups := make(map[string][]byte) | ||
| // Group metrics by name and serialize them | ||
| for _, m := range metrics { | ||
| tableName := m.Name() | ||
| metricInBytes, err := adx.serializer.Serialize(m) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if existingBytes, ok := tableMetricGroups[tableName]; ok { | ||
| tableMetricGroups[tableName] = append(existingBytes, metricInBytes...) | ||
| } else { | ||
| tableMetricGroups[tableName] = metricInBytes | ||
| } | ||
| } |
There was a problem hiding this comment.
Serialization and grouping should happen in the plugin(s) if different data-formats should be supported!
| func (adx *AzureDataExplorer) SetSerializer(serializer telegraf.Serializer) { | ||
| adx.serializer = serializer | ||
| } | ||
|
|
|
|
||
| var sampleConfig string | ||
|
|
||
| type AzureDataExplorer struct { |
There was a problem hiding this comment.
Usually we do have a config here and a "factory" function to create a client instance from it. So I would call this Config and have a function to create aClient from it with no exported fields...
There was a problem hiding this comment.
Please split the PR into one for azure_data_explorer and one for event_hubs!
| kustoerrors "github.com/Azure/azure-kusto-go/kusto/data/errors" | ||
| "github.com/Azure/azure-kusto-go/kusto/ingest" | ||
| "github.com/Azure/azure-kusto-go/kusto/kql" | ||
| adx_commons "github.com/influxdata/telegraf/plugins/common/adx" |
There was a problem hiding this comment.
Please use the common_adx as alias as we are following this pattern for other "common" packages too.
There was a problem hiding this comment.
Please keep the unit-tests as they were before to ensure the overall plugin mechanism works! Testing internal functions in common/adx is fine but the ones using the plugin interfaces (like Write) should stay as they are!
Summary
Checklist
Related issues
resolves #16371