supabase / supavisor

A cloud-native, multi-tenant Postgres connection pooler.
https://supabase.github.io/supavisor/
Apache License 2.0
1.76k stars 57 forks source link

Add Open Telemetry support #93

Open chasers opened 1 year ago

chasers commented 1 year ago
abc3 commented 1 year ago

Why do we need support OTEL, given that we already provide metrics in the Prometheus format?

J0 commented 1 year ago

Not familiar with surrounding context but based on this article it sounds like Prometheus metrics is a subset of OpenTelemetry Metrics so if I had to guess maybe some customers were looking for features which were in OTEL but not PromEx

Maybe we can check with @chasers once he is back, guessing he might have more context

chasers commented 1 year ago

With lots of tenants we can only have so many (very few) metrics before the endpoint Prometheus scrapes gets HUGE.

e.g. the Realtime endpoint is like 10MB at the moment

OTEL gets us much more visibility into everything if instrumented well and we can get full traces of connects, and potentially everything happening in the wire protocol (not required).

abc3 commented 1 year ago

gotcha 👍

J0 commented 1 year ago

Makes sense, will touch up on the PR over the weekend and get back

abc3 commented 1 year ago

@J0 The only thing is, could you make this optional? For example, use dependencies and only start using it when some environment variable, like ENABLE_OTEL=true, is passed

J0 commented 1 year ago

@abc3 Sure thing, will make it optional. Haven't figured out the specifics of how to do it but was thinking maybe we can define an OTEL build target and then import OTEL dependencies etc only ifMix.target() == :otel or something.

Let me know if y'all have any preferences or suggestions

abc3 commented 1 year ago

@J0 don't have any preferences, up to you

J0 commented 1 year ago

Hey

@abc3 am a little stuck on the conditional imports - seems like Dialyzer will complain if the imports are conditional since the Application start will be relying on dependencies that won't be bundled in when Mix.Target() is not set to OpenTelemetry Wondering if y'all have any suggestions there with respect to conditional imports.

Alternatively, is there any chance we can keep the Opentelemetry dependencies but continue to only start using it when some environment variable, like ENABLE_OTEL=true, is passed?

abc3 commented 1 year ago

Wondering if y'all have any suggestions there with respect to conditional imports.

you can use macros or ignore warnings through @dialyzer module attribute

Alternatively, is there any chance we can keep the Opentelemetry dependencies but continue to only start using it when some environment variable, like ENABLE_OTEL=true, is passed?

I would prefer to avoid that approach

J0 commented 1 year ago

Gotcha, thanks! Will try that in the evening or early tmrw

J0 commented 1 year ago

Hey,

Wanted to circle back on this - I wasn't able to figure out how to properly make the imports conditional. If anyone wants to jump on this please feel free - otherwise will look again in the future whenever a slot frees up

janpio commented 1 year ago

Original issue descriptions seems to be about spans (so Otel tracing), while discussion then pivoted to metrics. Was that intentional?

Otel spans / telemetry would be very interesting to be able to add deep understanding to each query that is being handled.

chasers commented 1 year ago

@janpio no, this is still re: implementing Otel tracing along the query pipeline.

We have already implemented a lot of metrics which are exposed at /metrics (for the whole system) and /metrics/:external_id (filtered per tenant).

dkuku commented 9 months ago

As this is a topic about opentelemetry I will ask here: Did you consider adding sqlcommenter support - this is a part of opentelemetry.

ConProgramming commented 7 months ago

Think this would be huge. An offering like highlight.io inside Supabase would be incredible.