scoutapp / scout_apm_elixir

ScoutAPM Elixir Agent. Supports Phoenix and other frameworks.
https://scoutapm.com
Other
36 stars 20 forks source link

Add option to control whether the app name is trimmed from controllers #111

Closed schneiderderek closed 4 years ago

schneiderderek commented 4 years ago

Currently, the way the library is setup controller instrumentation reports controller names sans the app name. As an example, in a standard Phoenix app if I have a controller called ExampleApp.SessionController then it will be reported under SessionController#action_name. This is very convenient for apps that only have one phoenix app since it reduces verbosity.

However, we're running Scout in an umbrella app that has multiple Phoenix apps underneath it. So in our setup we might have something that looks like this: ExampleApp1.SessionController and ExampleApp2.SessionController. With the current implementation, instrumentation of both controllers results in metrics being sent to the same SessionController#action_name which is undesirable because we would like them to be reported separately under their respective app names.

In order to retain the existing behavior and avoid introducing a change that would cause issues with anyone's existing metrics I've introduced a configuration option that controls whether or not the app module name is trimmed. By default the value is true indicating that the app module name will be trimmed.

cschneid commented 4 years ago

Thank you for the PR! I'll have @mitchellhenke take a look.

The other option would be to add a call to rename in the overlapping controllers if it's just a few cases.

https://github.com/scoutapp/scout_apm_elixir/blob/master/lib/scout_apm/tracked_request.ex#L169-L171

schneiderderek commented 4 years ago

Thank you for the PR! I'll have @mitchellhenke take a look.

The other option would be to add a call to rename in the overlapping controllers if it's just a few cases.

https://github.com/scoutapp/scout_apm_elixir/blob/master/lib/scout_apm/tracked_request.ex#L169-L171

@cschneid Thanks for the suggestion! I didn't know this existed. The way our umbrella app is setup we've got quite a few that would overlap with each other and i'd worry about situations in the future where we might add another controller that overlaps with an existing one and not realize/forget to override the transaction name there.

It'd be awesome if we could avoid doing that and bake the it in a bit more, but if we can't then overriding the transaction name is a good backup.

Also, this was only my first pass on this feature so i'm happy to make whatever adjustments/changes are needed.

schneiderderek commented 4 years ago

@mitchellhenke I've applied all of your suggestions, thank you for taking a look at this! 😄

I did have one additional question, we're running into a similar issue with the template names/paths that are sent over to Scout. The app directory that the template is in is being cutoff. As an example: instead of app_name/templates/resource/template_name.eex it's being shortened to just templates/resource/template_name.eex which makes it difficult for us to track down templates between applications. Looks like that's happening here.

Is that something I can address as well? If so would you want to do that in this PR or a separate PR/issue?

mitchellhenke commented 4 years ago

@schneiderderek awesome, thank you! It absolutely makes sense to fix it in the templates as well. I think including in this PR would be fine with the issues being related and the fix is more straightforward.

The template path isn't used for grouping or anything, so we can change to always return the full path directly without trying to drop certain parts. Ex:

ScoutApm.Tracing.timing("EEx", unquote(path), [scopable: !unquote(is_layout)],

It should also be updated in the ExsEngine

schneiderderek commented 4 years ago

@mitchellhenke 👍 sounds good. I went ahead and updated those two and the Slime engine since it looked similar.

I was able to remove the scout_name code in the Exs engine, but not the other two. Was that expected? It looked like the scopable option/is_layout code depends on that name.

Thanks again for taking a look at this!

mitchellhenke commented 4 years ago

@schneiderderek yep!

this looks great, thanks for the patience in addressing my questions!

schneiderderek commented 4 years ago

@mitchellhenke Would it be possible to create a new release of scout_apm with these changes so we can update our package without having to use a GH branch/ref?

mitchellhenke commented 4 years ago

@schneiderderek yep, I've just released 1.0.6!

schneiderderek commented 4 years ago

@mitchellhenke Awesome, I appreciate it! 😃