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.

Sign up for GitHub

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

Jump to bottom

AppSignal active configuration detection #86

Closed
tombruijn opened this issue Jan 19, 2017 · 4 comments
Closed

AppSignal active configuration detection #86

tombruijn opened this issue Jan 19, 2017 · 4 comments
Assignees
@tombruijn
Milestone
Elixir 1.0

Comments

@tombruijn
Copy link
Member

tombruijn commented Jan 19, 2017 •
edited

I found a difference in configuration between the AppSignal for Elixir package and the AppSignal for Ruby gem.

Related issue: #49

Ruby

In the Ruby gem we automatically set active to true once we detect the APPSIGNAL_PUSH_API_KEY in the environment.

This is the exception to the configuration order which allows us to install the Heroku add-on without setting the APPSIGNAL_ACTIVE environment key there. (It's too late to change that now.)

https://github.com/appsignal/appsignal-ruby/blob/0e97b6f9180c4504e4361021fafa6ec7da3e6d42/lib/appsignal/config.rb#L154-L155

Elixir

In the Elixir package we automatically set active to true if the push_api_key configuration option is set in any of the configuration stages (app_config and env_config) even when active is not explicitly set to true.

appsignal-elixir/lib/appsignal/config.ex

Line 37 in f7fd3f7

|> Map.put(:active, (if config[:active] == nil, do: !empty?(config[:push_api_key]), else: config[:active]))

Suggested change

We should try to keep the two packages' configuration behave the same way.

We should add the APPSIGNAL_PUSH_API_KEY active detection to the load_from_system function in the Elixir package. This require users to configure active per environment.

# config/config.exs
config :appsignal, :config,
  name: :appsignal_phoenix_example,
  push_api_key: "xxxx-xxxx-xxxx-xxx"
# config/dev.exs
config :appsignal, :config,
  env: :development,
  active: false # Default value so it's not necessary to set this
# config/prod.exs
config :appsignal, :config,
  env: :production,
  active: true

@jeffkreeftmeijer @arjan WDYT?
@thijsc We've discussed this behavior before for the Ruby gem, do you agree it should behave the same here?

Sidenote: Can't we detect the env from Mix.env ? So it's not necessary to set? See #91.
@tombruijn tombruijn added this to the Elixir 1.0 milestone Jan 19, 2017
@tombruijn tombruijn changed the title AppSignal active configuration AppSignal active configuration detection Jan 19, 2017
@jeffkreeftmeijer
Copy link
Member

I'm +1 on keeping the configuration the same in the Ruby and Elixir integrations, but I think I prefer having the :active option set to true by default. That way, we can turn Appsignal off for certain environments if needed, instead of explicitly turning it on. Why did we chose to do it the other way around for the Ruby version, @thijsc?

If we'd follow the Ruby way here (not activating the extension by default and having the user do so explicitly), we need to think of a good upgrade path for existing Elixir integration users.

@jeffkreeftmeijer jeffkreeftmeijer mentioned this issue Jan 25, 2017
Closed
@tombruijn tombruijn mentioned this issue Feb 3, 2017
Closed
2 tasks
@thijsc
Copy link
Member

thijsc commented Feb 6, 2017

I don't really get what the issue is. In Ruby active will be set to true automatically if there is a push key. You can disable it manually later on if you like. What would the backwards incompatibility be, people currently already need a key anyway?

@thijsc
Copy link
Member

thijsc commented Feb 6, 2017

Discussed with @tombruijn, this is indeed different from the Ruby version. I think we should keep the behaviour the same as in Ruby, otherwise providing support on this will be very confusing.

@tombruijn
Copy link
Member Author

I'll change it then to match the Ruby gem setup. A guide will be in order for everyone that upgrades

@tombruijn tombruijn mentioned this issue Feb 8, 2017
Merged
tombruijn added a commit that referenced this issue Feb 8, 2017
@tombruijn
Auto activate when push_api_key in env, not always
3677139 
Don't activate AppSignal always, when the push_api_key is in any config
type (system, app, env), but only activate AppSignal by default when the
push_api_key is found in the system environment variable
APPSIGNAL_PUSH_API_KEY.

This change makes the activation behavior the same for Elixir as for our
Ruby gem (https://github.com/appsignal/appsignal-ruby), which is more
flexible.

As discussed in #86

What this change means in practice. Before:

```
config :appsignal, :config,
  name: :my_app,
  push_api_key: "your-hex-appsignal-key"
```

This would make AppSignal active _always_, as long as the push_api_key
was set. The only exception was when `active` was explicitly set to
`false`.

After this change you need to explicitly set `active: true` to activate
AppSignal.

```
// config/config.exs
config :appsignal, :config,
  name: :my_app,
  active: true,
  push_api_key: "your-hex-appsignal-key"
```

Or as is probably be more common, activation per environment:

```
// config/config.exs
config :appsignal, :config,
  name: :my_app,
  push_api_key: "your-hex-appsignal-key"
```

```
// config/prod.exs
config :appsignal, :config,
  active: true
```

The exception to this rule is the APPSIGNAL_PUSH_API_KEY environment
variable. When this environment variable is found in the system
environment AppSignal also becomes active: `active: true`.
Unless explicitly set to `active: false` in the application config or
in the environment `APPSIGNAL_ACTIVE=false`.
jeffkreeftmeijer pushed a commit that referenced this issue Feb 14, 2017
@tombruijn @jeffkreeftmeijer
Auto activate when push_api_key in env, not always
05021bc 
Don't activate AppSignal always, when the push_api_key is in any config
type (system, app, env), but only activate AppSignal by default when the
push_api_key is found in the system environment variable
APPSIGNAL_PUSH_API_KEY.

This change makes the activation behavior the same for Elixir as for our
Ruby gem (https://github.com/appsignal/appsignal-ruby), which is more
flexible.

As discussed in #86

What this change means in practice. Before:

```
config :appsignal, :config,
  name: :my_app,
  push_api_key: "your-hex-appsignal-key"
```

This would make AppSignal active _always_, as long as the push_api_key
was set. The only exception was when `active` was explicitly set to
`false`.

After this change you need to explicitly set `active: true` to activate
AppSignal.

```
// config/config.exs
config :appsignal, :config,
  name: :my_app,
  active: true,
  push_api_key: "your-hex-appsignal-key"
```

Or as is probably be more common, activation per environment:

```
// config/config.exs
config :appsignal, :config,
  name: :my_app,
  push_api_key: "your-hex-appsignal-key"
```

```
// config/prod.exs
config :appsignal, :config,
  active: true
```

The exception to this rule is the APPSIGNAL_PUSH_API_KEY environment
variable. When this environment variable is found in the system
environment AppSignal also becomes active: `active: true`.
Unless explicitly set to `active: false` in the application config or
in the environment `APPSIGNAL_ACTIVE=false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tombruijn tombruijn

Labels
None yet
Projects
None yet
Milestone
Elixir 1.0
Development

No branches or pull requests
4 participants
@thijsc @matsimitsu @jeffkreeftmeijer @tombruijn