Emit source and sink types on app start #4105
Conversation
cli/pkg/local/app.go
Outdated
| // Collect and emit information about registered source types | ||
| err = emitSourceTelemetry(ctx, opts, rt, inst) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This is currently a blocking operation with a network call that will hold back starting of the application. I think we should run it in a goroutine.
cli/pkg/local/app.go
Outdated
| @@ -624,6 +632,49 @@ func initLogger(isVerbose bool, logFormat LogFormat) (logger *zap.Logger, cleanu | |||
| } | |||
| } | |||
|
|
|||
| func emitSourceTelemetry(ctx context.Context, opts *AppOptions, rt *runtime.Runtime, inst *drivers.Instance) error { | |||
There was a problem hiding this comment.
Would suggest renaming it to reflect that it emits a start event (the choice of start attributes may increase in the future). Also, these are pretty stateful objects to pass into a util function. Would suggest making it a member function:
func (a *App) emitStartEvent(ctx context.Context) error
cli/pkg/local/app.go
Outdated
| controller, err := rt.Controller(ctx, inst.ID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| catalog, _, err := rt.Catalog(ctx, inst.ID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| resources, err := catalog.FindResources(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| var sourceDrivers []string | ||
| var olapDrivers []string | ||
| for _, res := range resources { | ||
| if res.Kind == "rill.runtime.v1.Source" { | ||
| source, err := controller.Get(ctx, &runtimev1.ResourceName{Kind: res.Kind, Name: res.Name}, false) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| spec := source.GetSource().Spec | ||
| sourceDrivers = append(sourceDrivers, spec.SourceConnector) | ||
| olapDrivers = append(olapDrivers, spec.SinkConnector) | ||
| } | ||
| } |
There was a problem hiding this comment.
This has a problem, which is that the catalog is often not correctly populated right when the application starts (needs to wait for reconcile).
Instead, it would be better to use the AnalyzeConnectors function on the project parser, which is designed to explicitly extract info about the connectors used in the project. You can see the code for the rill env configure command for an example.
Note that the AnalyzeConnectors function covers everything, not just sources. I think it might be better just to emit the names of all connectors and then separately emit the name of the main OLAPConnector.
cli/pkg/local/app.go
Outdated
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| _ = tel.Flush(ctx) // Ignore error and start the app |
There was a problem hiding this comment.
Log the error at debug level instead of ignoring
cli/pkg/telemetry/telemetry.go
Outdated
| SourceDriver []string `json:"source_driver"` | ||
| OLAPDriver []string `json:"olap_driver"` |
There was a problem hiding this comment.
Can we use a generic Payload map[string]any or something like that instead? Unlike the other fields, these properties seem specific to a single event
There was a problem hiding this comment.
It depends on how druid parses nested multi-value dimensions
We can also mix in additional dimensions
val additionalDims map[string]any
// Convert struct to map and merge with additional data
var resultMap map[string]interface{}
temp, _ := json.Marshal(fields)
json.Unmarshal(temp, &resultMap)
for key, value := range additionalDims {
resultMap[key] = value
}
// Marshal the combined data to JSON
event, _ := json.Marshal(resultMap)
There was a problem hiding this comment.
isolated this into a separate commit
cli/pkg/local/app.go
Outdated
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
Since it's emitted in the background, maybe we don't need the timeout?
cli/pkg/telemetry/telemetry.go
Outdated
| } | ||
|
|
||
| func (t *Telemetry) EmitStartEvent(sourceDrivers []string, olapDriver string) { | ||
| payload := map[string]any{"source_driver": sourceDrivers, "olap_driver": olapDriver} |
There was a problem hiding this comment.
The analyzed connectors actually not necessarily just for sources, e.g. if we add support for exporting to a connector, it would also show up here. So would recommend renaming sourceDrivers to just connectors.
And maybe just to align terminology, also consider changing olap_driver to olap_connector.
* Emit source and sink types on app start * Parser vs catalog * Payload * Renamed payload props
No description provided.