romul / newrelic.ex

NewRelic agent for Elixir
MIT License
64 stars 19 forks source link

Enables reported language to be configurable and delayed naming of transaction #3

Closed jolson88 closed 7 years ago

jolson88 commented 7 years ago

We ran into some issues we needed to fix when using this library in our own code-base:

Feedback is welcome of course :).

romul commented 7 years ago

Hello, @jolson88! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

jolson88 commented 7 years ago

Hey @romul! Is there anything I can do or need to do to help out with getting this merged if possible? I'd love to switch our code back to using your official library vs. our own custom fork :).

romul commented 7 years ago

Hey @jolson88! Thank you for the contributions and sorry for the late reply. I've started to move your commits to the master. As to name function.. I'd prefer to see it more like

  @spec name(t, String.t) :: t
  def update_name(transaction, new_name) do
    %{transaction | name: new_name}
  end

In this case: the function name is more specific, the first arg - is the main one (Elixir convention) and there's no need in destructuring of transaction (maybe will be more fields in this struct in the future).

romul commented 7 years ago

Btw, I'm ok with making phoenix dependency optional. So help in this direction is also appreciated.

jolson88 commented 7 years ago

I like those ideas! I'll go ahead and make the changes later today.

jolson88 commented 7 years ago

Okay, changes made and pushed back up. Thanks again!

romul commented 7 years ago

@jolson88 Merged! And the version 0.1.1 with your changes has been also released. Thank you again for the help.

jolson88 commented 7 years ago

Thanks romul! You betcha!