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

Add telemetry events around the HTTP calls. #91

Merged
merged 3 commits into from
Jun 14, 2020

Conversation

aselder
Copy link
Contributor

@aselder aselder commented Jun 11, 2020

This add optional Telemetry events around the HTTP calls to BrainTree. See the README for the shape of the events.

Telemetry is added as an optional dependency, and the code will skip the telemetry calls if the telemetry application is not running

Copy link
Owner

@sorentwo sorentwo left a comment

Choose a reason for hiding this comment

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

This looks really solid, nicely done! I’m requesting a couple of changes to reduce complexity and future-prof it a bit. Please let me know your thoughts.

lib/http.ex Outdated
case response do
catch
kind, reason ->
stacktrace = System.stacktrace()
Copy link
Owner

Choose a reason for hiding this comment

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

This is deprecated, we need to use __STACKTRACE__ instead. If the minimum elixir version is too low we can bump it as well.

lib/http.ex Outdated
end
end

defp telemetry_started do
Copy link
Owner

Choose a reason for hiding this comment

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

Telemetry is getting ubiquitous enough and is light weight enough that I think we can have it as a non-optional dependency.

aselder added 2 commits June 13, 2020 20:15
This means we need to bump the required minimum version of Elixir to 1.7
@sorentwo sorentwo merged commit 1dc0717 into sorentwo:master Jun 14, 2020
@sorentwo
Copy link
Owner

Awesome work, thanks!

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.

2 participants