Introduce minimal dependency analytics interface#72262
Conversation
e4b95dd to
1b52135
Compare
1b52135 to
a75027d
Compare
e11e13c to
b63aa21
Compare
| :refer [gsheets gsheets!]] | ||
| [metabase-enterprise.harbormaster.client :as hm.client] | ||
| [metabase.analytics.core :as analytics] | ||
| [metabase.analytics.interface :as analytics] |
There was a problem hiding this comment.
is this too unorthodox?
There was a problem hiding this comment.
Not to bikeshed, but yeah my expectation would be that the "public interface" namespace would be the top-level metabase.analytics.
There was a problem hiding this comment.
the issue here is that public API is usually metabase.<modul-name>.core and is required as <modul-name>. there is no convention how to deal with a separate interface namespace different from the core. I hijacked the standard analytics alias for the interface namespace, because in most cases that's what users should import.
There was a problem hiding this comment.
I think in this case we'd probably normally actually want to have a separate interface module, e.g. analytics-interface with an API namespace of metabase.analytics-interface.core
| (let [table-size (row-count pgvector gate-table-name)] | ||
| (log/debugf "Setting `semantic-gate-size` metric to %d" table-size) | ||
| (analytics/set! :metabase-search/semantic-gate-size table-size) | ||
| (analytics/set-gauge! :metabase-search/semantic-gate-size table-size) |
There was a problem hiding this comment.
I didn't feel like fighting the set! special form, especially with CLJS added to the picture.
| [metabase.analytics.core :as analytics.core] | ||
| [metabase.analytics.interface :as analytics] |
There was a problem hiding this comment.
here we have both namespaces.
| @@ -0,0 +1,19 @@ | |||
| (ns metabase.analytics.impl | |||
There was a problem hiding this comment.
I think impl is fine, but you could also call it metabase.analytics.reporter.
| @@ -1,4 +1,5 @@ | |||
| (ns metabase.analytics.init | |||
| (:require | |||
| [metabase.analytics.impl] | |||
There was a problem hiding this comment.
to register the reporter.
There was a problem hiding this comment.
It would be better to add a comment in the code to this effect 🙃
technomancy
left a comment
There was a problem hiding this comment.
Can't really comment on the frontend stuff, but the backend side makes sense.
camsaul
left a comment
There was a problem hiding this comment.
I think this all looks good but it would be my preference to split out analytics-interface into its own zero-dependency module and then have a separate analytics or analytics-impl module with the Prometheus/Snowplow/whatever implementations in it
e2e tests failed on
|
| File | Test Name |
|---|---|
model-actions.cy.spec.js |
Write actions on model detail page (postgres) > should respect impersonated permission |
source-replacement.cy.spec.ts |
scenarios > data-studio > source replacement > Native queries > replaces a table referenced in a native SQL question |
dependency-unreferenced-list.cy.spec.ts |
scenarios > dependencies > unreferenced list > analysis > should show unreferenced entities |
dependency-unreferenced-list.cy.spec.ts |
scenarios > dependencies > unreferenced list > filters > should filter entities by type |
dependency-unreferenced-list.cy.spec.ts |
scenarios > dependencies > unreferenced list > filters > should persist filter changes after page reload |
dependency-unreferenced-list.cy.spec.ts |
scenarios > dependencies > unreferenced list > filters > should filter by location |
dependency-unreferenced-list.cy.spec.ts |
scenarios > dependencies > unreferenced list > selecting entities > should show the sidebar for supported entities and trigger snowplow event |
* Lighter prometheus analytics There's a couple things about prometheus that have gotten a bit unwieldy. This pain has recently been seen in Tamas's change that broke startup in a very "action at a distance" way. #72262 is _excellent_ work. But prometheus starts up so late that it is hazardous. Principally it requires app db support (for the setting system) and it loads essentially last after app db is initialized, token checks, etc. And this is the bad part. Many systems can log analytic events before the system is initialized. There are calls that when incrementing a a metric, if it's not already initialized, to initialize it. But in the course of initializing, it would want to increment a metric, causing an infinite loop. This was exacerbated in that the safety valves of the token checker kicked in and suppressed errors for 30 seconds, ending up with threads locked up trying to initialize the collector. Solution: just initialize it. It's just a few atomic longs and maybe a small webserver. One complication is from chris's work in #52834 where they try to seed initial values so that when something registers a number it is a jump from 0 to 8 and therefore alarming, rather than just 8 with no differential. So i added an edge that these can be set later on in the process. One thing to call out is that the initial value is mostly 0, but the search ones use a value of 1 as a bit field to indicate it's true: ``` prometheus=> (initial-labelled-metric-values) [ ,,, {:metric :metabase-search/engine-default, :labels {:engine "appdb"}, :value 1} ;; -> indicates appdb is the default engine {:metric :metabase-search/engine-default, :labels {:engine "semantic"}, :value 0} ,,, {:metric :metabase-search/engine-active, :labels {:engine "in-place"}, :value 1} {:metric :metabase-search/engine-active, :labels {:engine "appdb"}, :value 1} {:metric :metabase-search/engine-active, :labels {:engine "semantic"}, :value 0} ``` So when we call the initial observed values it's just setting an affirmative 0 on the value, or an affirmative 1. we won't clobber the number of emails sent or anything * initialize prometheus in test runner * remove tests that care about reentrant and auto start * default no-op reporter * remove setting up dynamic var. we don't setup for you any more
Lighter prometheus analytics (#73456) * Lighter prometheus analytics There's a couple things about prometheus that have gotten a bit unwieldy. This pain has recently been seen in Tamas's change that broke startup in a very "action at a distance" way. #72262 is _excellent_ work. But prometheus starts up so late that it is hazardous. Principally it requires app db support (for the setting system) and it loads essentially last after app db is initialized, token checks, etc. And this is the bad part. Many systems can log analytic events before the system is initialized. There are calls that when incrementing a a metric, if it's not already initialized, to initialize it. But in the course of initializing, it would want to increment a metric, causing an infinite loop. This was exacerbated in that the safety valves of the token checker kicked in and suppressed errors for 30 seconds, ending up with threads locked up trying to initialize the collector. Solution: just initialize it. It's just a few atomic longs and maybe a small webserver. One complication is from chris's work in #52834 where they try to seed initial values so that when something registers a number it is a jump from 0 to 8 and therefore alarming, rather than just 8 with no differential. So i added an edge that these can be set later on in the process. One thing to call out is that the initial value is mostly 0, but the search ones use a value of 1 as a bit field to indicate it's true: ``` prometheus=> (initial-labelled-metric-values) [ ,,, {:metric :metabase-search/engine-default, :labels {:engine "appdb"}, :value 1} ;; -> indicates appdb is the default engine {:metric :metabase-search/engine-default, :labels {:engine "semantic"}, :value 0} ,,, {:metric :metabase-search/engine-active, :labels {:engine "in-place"}, :value 1} {:metric :metabase-search/engine-active, :labels {:engine "appdb"}, :value 1} {:metric :metabase-search/engine-active, :labels {:engine "semantic"}, :value 0} ``` So when we call the initial observed values it's just setting an affirmative 0 on the value, or an affirmative 1. we won't clobber the number of emails sent or anything * initialize prometheus in test runner * remove tests that care about reentrant and auto start * default no-op reporter * remove setting up dynamic var. we don't setup for you any more Co-authored-by: dpsutton <dan@dpsutton.com>
Master moved Prometheus helper fns (inc!, set!/set-gauge!, observe!) out of metabase.analytics.core and into metabase.analytics-interface.core (see #72262), and the rename broke the namespace-decl check on this branch. Point all metabase.mq.* files (and their tests) at the new namespace, rename set! → set-gauge!, and add analytics-interface to the mq module's :uses set.
Closes QUE2-507
Description
This PR introduces an interface namespace for emitting internal analytics events, aiming for
The event emission parts that are provided by the new interface are migrated from
analytics.coreandprometheusto the new interface namespace.How to verify
The tests should pass.
Checklist
If adding new Loki tests: they pass stress testing