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

Reduce the number of dependencies #1

Closed

Conversation

arkgil
Copy link

@arkgil arkgil commented Jun 3, 2022

Hey folks 👋 It's really great to see Elixir being supported as one of the first languages for Test Analytics!

I stumbled into a bunch of dependency conflicts when installing the collector, and I found that that telemetry is not used at all, and Ecto is only used for UUID generation. I removed telemetry altogether and replaced Ecto with a dedicated UUID library.

Let me know what you think!

@@ -30,7 +29,7 @@ defmodule BuildkiteTestCollector.TestResult do

@typedoc "Individual test summary. Spec as yet unconfirmed."
@type t :: %TestResult{
id: UUID.t(),
id: String.t(),
Copy link
Author

Choose a reason for hiding this comment

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

Made it a string, but I can also create a custom type for the UUID if you prefer that.

@joshprice
Copy link
Collaborator

joshprice commented Jun 3, 2022

Thanks so much for the PR! 👍🏻

We'll be adding telemetry for Ecto shortly, so although we don't need these now, we will very shortly.

We also explicitly chose Ecto's UUID because https://github.com/zyro/elixir-uuid is not currently maintained. It's last commit was 3 years ago and there are many unmerged PRs and unaddressed issues, like this one zyro/elixir-uuid#60

What were the dependency conflicts you ran into?

@joshprice
Copy link
Collaborator

joshprice commented Jun 3, 2022

Just found a thread on the UUID package (zyro/elixir-uuid#50) which mentions a potential UUID alternative we could investigate

https://github.com/bitwalker/uniq

That said, if we're using Ecto anyway, there's probably not much point in adding another dependency.

@jimsynz
Copy link
Collaborator

jimsynz commented Jun 3, 2022

@joshprice we could perhaps make it an optional dependency?

@arkgil
Copy link
Author

arkgil commented Jun 3, 2022

Ah, that makes sense. Monitoring Ecto in tests sounds pretty interesting!

One thing you could consider is relaxing the requirement on Telemetry. There are no API changes between version 0.4 and 1.0, so having the ability to use either of the two would definitely help with adoption. See e.g. Phoenix: https://github.com/phoenixframework/phoenix/blob/0336846f27cb366c76c2b03f2bca220d25a56e08/mix.exs#L71, and Ecto: https://github.com/elixir-ecto/ecto/blob/1f04999ab2647115d75140d35840b30e40be60db/mix.exs#L35.

@arkgil
Copy link
Author

arkgil commented Jun 3, 2022

Actually, relaxing the requirement on Ecto version would help too, since 3.8 is pretty recent.

@joshprice
Copy link
Collaborator

If you want to update the PR to relax the version requirements instead, would be happy to merge. UUID has been there since 3.2 but let's say ~> 3.6 for telemetry decent telemetry support?

@arkgil
Copy link
Author

arkgil commented Jun 3, 2022

@joshprice 3.4.1 was the latest release that introduced a new event (ecto.repo.init): https://github.com/elixir-ecto/ecto/blob/master/CHANGELOG.md#v341-2020-04-08. 3.8.0, however, added stacktraces to the events. Not sure if you plan to use those or not.

@joshprice
Copy link
Collaborator

Closing this PR. As @jimsynz pointed out we'll look at making the deps optional and relaxing the versions as much as possible, as not everyone will be tracing SQL. Thanks again for the valuable feedback, most appreciated! 🙏🏻

@joshprice joshprice closed this Jun 8, 2022
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.

3 participants