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.

Sign up for GitHub

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

Jump to bottom

feat(streaming): raw event connector #1720

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(streaming): raw event connector #1720

wants to merge 2 commits into from

Conversation

hekike
Copy link
Contributor

@hekike hekike commented Oct 21, 2024 •
edited
Loading

As of today, OpenMeter uses ClickHouse materialized views to track meters. Events are inserted into a raw events table on which MVs are based. This PR moves the event insert logic out from the sink worker to the streaming connector and splits the connector into two different connectors:

  1. Raw events connector: a low-level connector inserts and queries only raw events. Using this connector is expensive to query meters, as will JSON parse each event.
  2. Materialized View connector that extends raw event connector to speed up querying meters. (current default)

PR review guide:

  • ./openmeter/streaming/clickhouse/materialized_view/meter_query and test is unchanged just moved around
  • ./openmeter/streaming/clickhouse/raw_events/event.query and test is unchanged; just moved around

@hekike hekike added area/ingest release-note/feature Release note: Exciting New Features labels Oct 21, 2024
@hekike hekike changed the title feat(streaming): poc feat(streaming): raw event connector Oct 21, 2024
@hekike hekike marked this pull request as ready for review October 21, 2024 19:25
sagikazarmark
sagikazarmark reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Why did you start passing meter models.Meter to some functions and kept meterSlug in others? Why start doing it at all? Can't the connector query the meter from the repository?

app/common/openmeter.go Outdated Show resolved Hide resolved
app/config/aggregation.go Outdated Show resolved Hide resolved
app/config/aggregation.go Show resolved Hide resolved
cmd/server/main.go Outdated Show resolved Hide resolved
openmeter/meter/parse.go Show resolved Hide resolved
@hekike hekike force-pushed the feat/meter-event branch 2 times, most recently from 1b4b83e to 7a1e5cc Compare October 23, 2024 01:53
@hekike hekike force-pushed the feat/meter-event branch 2 times, most recently from df8f6eb to 7b17e96 Compare November 15, 2024 21:16
hekike
hekike commented Nov 15, 2024
View reviewed changes
app/config/aggregation.go
@@ -100,6 +175,11 @@ func (c ClickHouseAggregationConfiguration) GetClientOptions() *clickhouse.Optio

// ConfigureAggregation configures some defaults in the Viper instance.
func ConfigureAggregation(v *viper.Viper) {
v.SetDefault("aggregation.engine", AggregationEngineClickHouseMV)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably we should change default to raw events

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagikazarmark sagikazarmark sagikazarmark left review comments

@chrisgacsal chrisgacsal Awaiting requested review from chrisgacsal

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/ingest release-note/feature Release note: Exciting New Features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hekike @sagikazarmark @chrisgacsal