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

sync-service: only include telemetry dependencies and processes in application target build #2379

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

magnetised
Copy link
Contributor

configure telemetry dependencies to only be included when building the application target and do compile-time checks to exclude certain modules when compiling without telemetry

uses both checks against Mix.target() and checks for the presence of dependent modules to make compilation and runtime robust when running as docker application, local dev server and when used as a dependency

having said that, a better solution would be to move the telemetry to a separate library and have it included when needed but this solves the main problem of un-wanted telemetry dependencies being included (and needing configuration) when electric is included as a dependency

@magnetised magnetised force-pushed the magnetised/phoenix-sync-5-optional-telemetry branch from bd7a325 to 89959a6 Compare February 25, 2025 16:11
Copy link
Contributor

@robacourt robacourt left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me given all the constraints. As you say, this is highlighting that there's stuff cloud and stand-alone needs that's not needed by embedded, so a separate library makes sense at some point. But this seems reasonable for now.

@magnetised
Copy link
Contributor Author

dear reviewers, the tests are currently failing because, though I thought I'd made every pr self-contained, this actually has a dependency on #2375
if you can review in spirit, I'll ensure the tests pass after merging that pr and rebasing this one onto it

@balegas
Copy link
Contributor

balegas commented Feb 25, 2025

It seems that in embedded mode, people might still want to get spans for Electric (maybe with different level of detail). Can't we offer a way to opt-in for them? like https://hexdocs.pm/bandit/Bandit.Telemetry.html

I agree that Sentry is probably a concern of the standalone, but turning off usage telemetry would prevent us from knowing who's using Electric in embedded mode. Is there good reasons not to have them as an opt-out configuration as we have now?

@KyleAMathews
Copy link
Contributor

@balegas I don't think this is disabling otel

@magnetised
Copy link
Contributor Author

It seems that in embedded mode, people might still want to get spans for Electric (maybe with different level of detail). Can't we offer a way to opt-in for them? like hexdocs.pm/bandit/Bandit.Telemetry.html

This doesn't turn off the creation of spans, merely the sending of those spans and collection of other metrics data such as cpu and memory usage.

So if the developer wants to collect that info they can.

I think the ultimate plan should be to move the telemetry stuff to a separate package, which we include in our docker builds and internal apps, but isn't included by default in the electric elixir package. Then if a developer wanted, or needed, to send the telemetry they could just include that telemetry dependency.

This is just a quick-ish fix to remove the telemetry-by-default approach which was just resulting in weird errors.

I agree that Sentry is probably a concern of the standalone, but turning off usage telemetry would prevent us from knowing who's using Electric in embedded mode. Is there good reasons not to have them as an opt-out configuration as we have now?

I think the ultimate decision is beyond my pay grade but as a developer an open-source library that transmits usage data is a big no.

Also, if we wanted to collect this info from every install of the electric elixir package we'd need to include our sentry and honeycomb api keys in a public package, which is -- I think -- a phenomenally bad idea.

@balegas
Copy link
Contributor

balegas commented Feb 25, 2025

I agree with all. Was just not clear if was not possible to enable the span generation. I think that should be easy to enable even if you're using library mode, but everything else I agree only makes sense for the standalone.

…plication target build

configure telemetry dependencies to only be included when building the
`application` target and do compile-time checks to exclude certain
modules when compiling without telemetry

uses both checks against Mix.target() and checks for the presence of
dependent modules to make compilation and runtime robust when running as
docker application, local dev server and when used as a dependency

having said that, a better solution would be to move the telemetry to a
separate library and have it included when needed
@magnetised magnetised force-pushed the magnetised/phoenix-sync-5-optional-telemetry branch from 89959a6 to 70c78c1 Compare February 26, 2025 09:42
@magnetised magnetised merged commit 5da8f25 into main Feb 26, 2025
34 checks passed
@magnetised magnetised deleted the magnetised/phoenix-sync-5-optional-telemetry branch February 26, 2025 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants