Replace direct telemetry deps with the electric_telemetry library#3412
Replace direct telemetry deps with the electric_telemetry library#3412
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3412 +/- ##
==========================================
+ Coverage 67.82% 75.35% +7.53%
==========================================
Files 184 51 -133
Lines 9955 2743 -7212
Branches 405 409 +4
==========================================
- Hits 6752 2067 -4685
+ Misses 3201 674 -2527
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Why not keep https://github.com/electric-sql/electric-telemetry as part of our mono-repo as another elixir project? Easier to access and work with. |
|
@msfstef Main reason is decoupling. Our primary use case for telemetry is in Electric Cloud. So if we're going to make any changes to the telemetry code, it'll be informed by Cloud and should be tested first in Cloud. All while Electric keeps using its own stable version of the lib. How can we achieve that?
Moving electric-telemetry into Electric's repo would work fine with the 2nd option but I'm against publishing our telemetry as a Hex package since it is only ever going to work as part of Electric. With the 1st option it becomes awkward: we cannot use electric-telemetry in Electric as a path dependency because then Electric will necessarily always use the latest version of it. And the alternative is to pull it from Github (i.e. |
alco
force-pushed
the
alco/electric-telemetry
branch
4 times, most recently
from
66f1c97 to
8fedf3d
Compare
There was a problem hiding this comment.
We have a bunch of tests still in place for these, and I suspect they are still passing because we're reusing the Electric.Telemetry namespace, whereas I think this should now be in ElectricTelemetry, and the tests should move to the new repo, no?
alco
force-pushed
the
alco/electric-telemetry
branch
3 times, most recently
from
e9756ff to
4f74858
Compare
commit: |
alco
force-pushed
the
alco/electric-telemetry
branch
9 times, most recently
from
732c6de to
93eed43
Compare
magnetised
left a comment
magnetised
left a comment
There was a problem hiding this comment.
Awesome. We should have done it like this right from the start.
| if Electric.telemetry_enabled?() do | ||
| # Disable the default telemetry_poller process since we start our own in | ||
| # `ElectricTelemetry.{ApplicationTelemetry, StackTelemetry}`. | ||
| config :telemetry_poller, default: false |
There was a problem hiding this comment.
could we set this config value as a default in packages/electric-telemetry/mix.exs?
There was a problem hiding this comment.
I don't know what you mean here.
The problem is that telemetry_poller gets started before electric_telemetry because it's a dependency. And it looks up the :default key in its app env at startup. So the key has to be set before telemetry_poller gets started and config/runtime.exs of the parent app is the only place to have this config in.
There was a problem hiding this comment.
You can add env configuration to the application callback in mix.exs: https://hexdocs.pm/elixir/1.19.3/Application.html#module-the-application-environment
If we put this required config there it would remove the need to add it at every point of use
There was a problem hiding this comment.
But that configuration is for the app owns the mix.exs. We cannot put arbitrary config for other apps, such as telemetry_poller, in there.
These settings must be set in Electric and in Cloud, so using a single function helps avoid discrepancies.
This way, the electric_telemetry dependency can be included or excluded as a whole. And conditional compilation is only relevant to Electric anyway.
The telemetry library doesn't actually use Sentry. Cloud has its own setup for it, so electric_telemetry is not the right place for it.
Once telemetry is enabled via MIX_TARGET=application, making individual deps optional only puts the burden on the parent app to include them directly. We want it the other way around: let the telemetry library define its deps and fetch them.
The way to enable electric_telemetry is to set MIX_TARGET=application. In Cloud it is always enabled anyway.
As suggested by Stefanos, this makes it less confusing which modules belong to which package.
This is mirror what's already been done for CallHomeReporter
This way they are easier to share between Electric and Cloud
It is now used by electric_telemetry
This version fixes an issue with unintentional overriding of OTLP headers
Chasing a bug in metric export via Otel...
alco
force-pushed
the
alco/electric-telemetry
branch
from
e414455 to
3b9522e
Compare
|
This PR has been released! 🚀 The following packages include changes from this PR: Thanks for contributing to Electric! |
3 participants
Extract telemetry code from Electric into a separate package. On top of that, the new package accepts additional user-provided periodic measurements and metric definitions for otel/prometheus exporters; both exporters now use the same list of metrics.
One thing that's been lost in this move is the
stack_idsentry tag inside the CallHomeRepoter process. Sentry is a dependency of Electric and there's no straightforward way of propagating it into CallHomeReporter which is a module in the electric_telemetry dependency now. I don't feel like it's a big loss because we set stack_id in the logger metadata for that process, as well as mention it in the process name and label.With this merged, we can delete the electric-sql/electric-telemetry repo.