feat(analytics): Analytics Feature #2725
Conversation
…act with many possible implementaions
|
This looks great! I have one question. How will |
| err := runMigrations(logger, connectionString) | ||
| if err != nil { | ||
| clickhouseErr = err | ||
| return | ||
| } | ||
|
|
||
| connection, err := connect(connectionString) | ||
| if err != nil { | ||
| clickhouseErr = err | ||
| return | ||
| } | ||
|
|
||
| conn = connection |
There was a problem hiding this comment.
We could probably establish the connection once, instead of reconnecting after performing migrations:
| err := runMigrations(logger, connectionString) | |
| if err != nil { | |
| clickhouseErr = err | |
| return | |
| } | |
| connection, err := connect(connectionString) | |
| if err != nil { | |
| clickhouseErr = err | |
| return | |
| } | |
| conn = connection | |
| conn, clickhouseErr = connect(connectionString) | |
| if clickhouseErr != nil { | |
| return | |
| } | |
| clickhouseErr = runMigrations(logger, conn) | |
| return |
There was a problem hiding this comment.
@GeorgeMac Got you. I was following the same pattern as the RDMS ones too but it does make sense to just use the connection once as you suggested.
|
I wasn't paying attention and meant to make that a comment, not an approved review. But I do still approve of this PR 😂 so not wrong. |
yquansah
changed the title
|
@yquansah I know I am a bit late with this request. Is it possible to add |
|
@erka You are not late at all!! That is actually what is going to happen once I open it up. I plan to use https://clickhouse.com/docs/en/sql-reference/data-types/map to essentially have "tags" per measured metric. |
internal/config/analytics.go
Outdated
| return clickhouse.Auth{ | ||
| Username: c.Username, | ||
| Password: c.Password, | ||
| Database: "flipt_analytics", |
There was a problem hiding this comment.
@yquansah just want to unresolve and go over this once more: Don't we need to make sure this is consistent with whatevere the database name is in the URL provided. Otherwise, if they try to provide their own name, this wont be valid. Maybe I am misunderstanding this option though.
| step.intervalStep, | ||
| ), | ||
| req.NamespaceKey, | ||
| req.FlagKey, |
There was a problem hiding this comment.
From a cursory glance I don't think what you have here is exploitable, because you have validated your inputs already (parsed them as timestamps, or normalized them into intervals and so on), but format strings and SQL is usually a recipe for injection.
This is just to say, if you can coerce some of these parameters into the placeholder syntax like you have for namespace and flag key, you should be further protected from these kinds of vulnerability.
https://go.dev/doc/database/sql-injection
| //nolint:gosec | ||
| stmt := fmt.Sprintf("INSERT INTO %s VALUES %s", counterAnalyticsTable, strings.Join(valuePlaceHolders, ",")) |
There was a problem hiding this comment.
Just a passing thought, nothing to really do for now.
TL;DR something to keep an eye on r.e. performance.
I imagine constantly rebuilding this query to be quite wasteful, particularly under load. I remember you saying their native client is more performant for inserts, so maybe we just don't sweat this until we eventually move to their other client. However, one way to mitigate this might be cache the insert query based on buffer size. To try and amortize the cost and avoid constantly reallocating this query.
I wonder if CH has a better way to prepare this query, so that the placeholders don't have to be repeated in this way too. That might be something their own client takes advantage of.
Aside: I looked at this first, because of the injection problem, and the nosec there made suspicious too. But I see you're building the placeholder query with this. Which itself is just a series of palceholders, no user inputs, so it is safe. Then you're passing the values via the placeholder on exec, which is good. So I can see why you step around the linting. (I am assuming the noline rule is upset here because of fmt stringing a SQL ).
There was a problem hiding this comment.
@GeorgeMac Yeah exactly! I tried to see if there was a better way to prepare this query. But you are right, we can look into that more native client.
I can place an issue in the project to do that. We can see if it has more features that is better suited for inserts.
| keyValue := []attribute.KeyValue{ | ||
| { | ||
| Key: "flipt.evaluation.response", | ||
| Value: attribute.StringValue(string(evaluationResponsesBytes)), |
There was a problem hiding this comment.
Here is a bit of a mad hack: https://pkg.go.dev/go.opentelemetry.io/otel/attribute#Stringer
You could add a String() string method to *analytics.EvaluationResponse that marshals out a Go string. Then instead of marshalling this to JSON and then unmarshalling again in the exporter, you could pass it here with attributes.Stringer and just cast it back to a *analytics.EvaluationResponse in the exporter.
Otel will take care of marshalling lazilly when it needs to go over the wire.
There was a problem hiding this comment.
@GeorgeMac Ahh I started to implement it this way just now and realized,
What is actually happening here is I am marshalling a []*EvaluationResponse to JSON instead of just a single *EvaluationResponse.
I chose to do it this way because it helps with the Batch implementation.
There was a problem hiding this comment.
Could you re-type that slice for the same ends?
type EvaluationResponses []*EvaluationResponse
func (r EvaluationResponses) String() string {
v, _ := json.Marshal(r)
return string(v)
}And then use analytics.EvaluationResponses{} instead of []*analytics.EvaluationResponse?
Either way, not important, just seeing if we can avoid marshalling back and forth.
…nullable columns for clickhouse
internal/config/analytics.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if options.Auth.Database != "flipt_analytics" { |
There was a problem hiding this comment.
i think we want to allow them to use whatever db name they choose
There was a problem hiding this comment.
@markphelps We could do that. They would have to create that DB beforehand. I guess we can clearly state that in our docs
There was a problem hiding this comment.
can we try and parse the db name from the connection string?
if its there, then we use that, if not then we use this default?
There was a problem hiding this comment.
Yeah I think we definitely want them to create it first themselves. Like we do with the relational DB.
cmd/flipt/migrate.go
Outdated
| } | ||
|
|
||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(&providedConfigFile, "config", "", "path to config file") | ||
| cmd.Flags().StringVar(&database, "database", "regular", "string to denote which database type to migrate") |
There was a problem hiding this comment.
| cmd.Flags().StringVar(&database, "database", "regular", "string to denote which database type to migrate") | |
| cmd.Flags().StringVar(&database, "database", "default", "string to denote which database type to migrate") |
internal/config/analytics.go
Outdated
| v.SetDefault("analytics", map[string]any{ | ||
| "enabled": "false", | ||
| "clickhouse": map[string]any{ | ||
| "enabled": "false", | ||
| "url": "", | ||
| }, |
There was a problem hiding this comment.
| v.SetDefault("analytics", map[string]any{ | |
| "enabled": "false", | |
| "clickhouse": map[string]any{ | |
| "enabled": "false", | |
| "url": "", | |
| }, | |
| v.SetDefault("analytics", map[string]any{ | |
| "storage.clickhouse": map[string]any{ | |
| "enabled": "false", | |
| "url": "", | |
| }, |
…e any database they want
chore: style analytics well
| } | ||
|
|
||
| valuePlaceHolders := make([]string, 0, len(responses)) | ||
| valueArgs := make([]interface{}, 0, len(responses)*9) |
There was a problem hiding this comment.
| valueArgs := make([]interface{}, 0, len(responses)*9) | |
| valueArgs := make([]interface{}, 0, len(responses)*COLUMNS) |
Could we make 9 a constant so its not a magic number
| ), | ||
| nowISO.getTimezoneOffset() | ||
| ), | ||
| timeFormat |
There was a problem hiding this comment.
would it be possible to show the evaluation timestamps in the format that the user selects in preferences? ie UTC vs local?
| return { | ||
| timestamps: getFlagEvaluationCount.data?.timestamps, | ||
| values: getFlagEvaluationCount.data?.values, | ||
| unavailable: fetchError?.status === 501 |
There was a problem hiding this comment.
another option is we could check the /meta/config endpoint to see if analytics are enabled before making the call and depending on a 501
I Could do this in a future PR though as it will require some redux work I think
|
One other thing I noticed is that the I'm gonna check the sqlite db to see if the same thing happens there
|
In flipt-io#2725 there were changes to z-index of header and Slideover and Modal is behind the header. It's visible in creating new token/namespace or in any modal dialog
In #2725 there were changes to z-index of header and Slideover and Modal is behind the header. It's visible in creating new token/namespace or in any modal dialog
This PR addresses the feature of mainly providing a uniform way of querying analytics through Flipt's API. We have been getting feature requests such as this for some time now, and our focus here is to provide a uniform contract that can be implemented by many different OLAP stores which then sits behind an API that the frontend can consume.
Main Features of this PR
analytics.protodefinition for giving an API on top of an analytics store