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.

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

feat(streaming): raw event connector #1720

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

feat(streaming): raw event connector #1720

wants to merge 33 commits into from

Conversation

hekike
Copy link
Contributor

@hekike hekike commented Oct 21, 2024

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
Copy link
Member

@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
// Engine is the aggregation engine to use
Engine AggregationEngine
ClickHouse ClickHouseAggregationConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Long term we should probably move the clickhouse config to top level and better organize engine configs.

This change only increases the chaos as it is with aggregation level, but clickhouse specific configs. :/

cmd/server/main.go Outdated Show resolved Hide resolved
)

// ParseEvent validates and parses an event against a meter.
func ParseEvent(meter Meter, ev event.Event) (*float64, *string, map[string]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is validate being replaced by parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new connector required to know value. It was always parsing, just before didn't return the parsed value. This file looks new but it's mostly renamed. I removed the new connector from the PR but would like to keep parsing this way as it helps testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ingest release-note/feature Release note: Exciting New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants