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 config/appsignal.rb config file #1324

Merged
merged 5 commits into from
Nov 11, 2024
Merged

Add config/appsignal.rb config file #1324

merged 5 commits into from
Nov 11, 2024

Conversation

tombruijn
Copy link
Member

@tombruijn tombruijn commented Oct 31, 2024

Add config/appsignal.rb config file

Add config/appsignal.rb config file support. This is a replacement for the config/appsignal.yml config file.

Load the config/appsignal.rb file automatically on Appsignal.start. For most apps, like Rails apps, this will work the same way as config/appsignal.yml does now.

If Appsignal.configure is called manually in the app, like a Rails initializer, the config/appsignal.rb file is not loaded. Instead, we recommend moving the Appsignal.configure call to the config/appsignal.rb file.

New behavior

With this change the config is loaded as described in these scenarios:

  • Scenario 1:
    • Appsignal.start is called.
    • No config is loaded yet.
    • The config/appsignal.rb file is present.
    • The config/appsignal.rb is required and configures the gem.
  • Scenario 2:
    • Appsignal.start is called.
    • No config is loaded yet.
    • The config/appsignal.rb file is present.
    • The config/appsignal.yml file is present.
    • The config/appsignal.rb is required and configures the gem using
      the Appsignal.configure helper.
    • The config/appsignal.yml is not loaded.
  • Scenario 3:
    • Appsignal.configure is called.
    • The config/appsignal.rb file is present.
    • The config/appsignal.rb file is not loaded.
    • Appsignal.start is called.
  • Scenario 4:
    • Appsignal.configure is called.
    • The config/appsignal.yml file is present.
    • The config/appsignal.yml file is loaded.
    • Appsignal.start is called.

This is more complex that I would have liked. In hindsight I shouldn't have made Appsignal.configure load the config/appsignal.yml file. That would have made it a bit more predictable. But I don't want Appsignal.configure in the config/appsignal.rb file to also load the config/appsignal.yml file, because then it just gets more confusing.

In the next major version I want to remove config/appsignal.yml support, so this is the first step.

App/root path

I remember some of our users configured the Config class's root_path to specify where to load the config file from. If an app configures itself via a config/appsignal.rb, this makes it impossible to customize the root path. For that purpose I've added the APPSIGNAL_APP_PATH environment variable to customize the config path. Which I've also used to make it easier to test the different scenarios.


Part of #1324

Move config load logic to its own method

Add a private method (that has to be publicly accessible within the gem itself), to load the config. This avoids duplication in the diagnose CLI when I update it to call this method as well.

Make it more safe by replicating the existing behavior: it will never return a nil object for the Config once started. If it started with an error or the config file is empty, initialize the config but set active to false.

Load config/appsignal.rb in diagnose CLI

Update the diagnose CLI to call the new Appsignal._load_config! PRIVATE method to initialize the config. This way we have less duplication of how the config is initialized and we can be sure it works the same in the diagnose CLI as well.

I've introduced a new PRIVATE _APPSIGNAL_CONFIG_FILE_ENV env var to set the environment, but only in the diagnose CLI context. It's one of the few ways we can pass the default environment as set by the CLI to whatever config object gets initialized in the config file.

I didn't use the APPSIGNAL_APP_ENV environment variable so the config option doesn't show up in the report as being set by an env var or overwrite it when the user has set it.

@tombruijn tombruijn self-assigned this Oct 31, 2024
tombruijn added a commit that referenced this pull request Nov 4, 2024
Add `config/appsignal.rb` config file support. This is a replacement for
the `config/appsignal.yml` config file.

Load the `config/appsignal.rb` file automatically on `Appsignal.start`.
For most apps, like Rails apps, this will work the same way as
`config/appsignal.yml` does now.

If `Appsignal.configure` is called manually in the app, like a Rails
initializer, the `config/appsignal.rb` file is not loaded. Instead, we
recommend moving the `Appsignal.configure` call to the
`config/appsignal.rb` file.

## New behavior

With this change the config is loaded as described in these scenarios:

- Scenario 1:
  - Appsignal.start is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` is required and configures the gem.
- Scenario 2:
  - When Appsignal.start is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.rb` is required and configures the gem using
    the `Appsignal.configure` helper.
  - The `config/appsignal.yml` is not loaded.
- Scenario 3:
  - When Appsignal.start is called.
  - When `Appsignal.configure` has been called beforehand.
  - The `config/appsignal.rb` file is not required.
- Scenario 4:
  - When Appsignal.configure is called.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.yml` file is loaded.
  - `Appsignal.start` is called.
- Scenario 5:
  - When Appsignal.configure is called.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.yml` file is loaded.
  - `Appsignal.start` is called.

This is more complex that I would have liked. In hindsight I shouldn't
have made `Appsignal.configure` load the `config/appsignal.yml` file.
That would have made it a bit more predictable. But I don't want
`Appsignal.configure` in the `config/appsignal.rb` file to also load the
`config/appsignal.yml` file, because then it just gets more confusing.

In the next major version I want to remove `config/appsignal.yml`
support, so this is the first step.

## App/root path

I remember some of our users configured the Config class's `root_path`
to specify where to load the config file from.
If an app configures itself via a `config/appsignal.rb`, this makes it
impossible to customize the root path. For that purpose I've added the
`APPSIGNAL_APP_PATH` environment variable to customize the config path.
Which I've also used to make it easier to test the different scenarios.

---

Part of #1324
@tombruijn tombruijn changed the base branch from config-activate-if-env to main November 4, 2024 14:39
tombruijn added a commit that referenced this pull request Nov 4, 2024
Add `config/appsignal.rb` config file support. This is a replacement for
the `config/appsignal.yml` config file.

Load the `config/appsignal.rb` file automatically on `Appsignal.start`.
For most apps, like Rails apps, this will work the same way as
`config/appsignal.yml` does now.

If `Appsignal.configure` is called manually in the app, like a Rails
initializer, the `config/appsignal.rb` file is not loaded. Instead, we
recommend moving the `Appsignal.configure` call to the
`config/appsignal.rb` file.

## New behavior

With this change the config is loaded as described in these scenarios:

- Scenario 1:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` is required and configures the gem.
- Scenario 2:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.rb` is required and configures the gem using
    the `Appsignal.configure` helper.
  - The `config/appsignal.yml` is not loaded.
- Scenario 3:
  - `Appsignal.configure` is called.
  - `Appsignal.start` is called.
  - The `config/appsignal.rb` file is not required.
- Scenario 4:
  - `Appsignal.configure` is called.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.yml` file is loaded.
  - `Appsignal.start` is called.
- Scenario 5:
  - `Appsignal.configure` is called.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` file is not loaded.
  - `Appsignal.start` is called.

This is more complex that I would have liked. In hindsight I shouldn't
have made `Appsignal.configure` load the `config/appsignal.yml` file.
That would have made it a bit more predictable. But I don't want
`Appsignal.configure` in the `config/appsignal.rb` file to also load the
`config/appsignal.yml` file, because then it just gets more confusing.

In the next major version I want to remove `config/appsignal.yml`
support, so this is the first step.

## App/root path

I remember some of our users configured the Config class's `root_path`
to specify where to load the config file from.
If an app configures itself via a `config/appsignal.rb`, this makes it
impossible to customize the root path. For that purpose I've added the
`APPSIGNAL_APP_PATH` environment variable to customize the config path.
Which I've also used to make it easier to test the different scenarios.

---

Part of #1324
@tombruijn tombruijn marked this pull request as ready for review November 4, 2024 15:16
tombruijn added a commit that referenced this pull request Nov 5, 2024
Add `config/appsignal.rb` config file support. This is a replacement for
the `config/appsignal.yml` config file.

Load the `config/appsignal.rb` file automatically on `Appsignal.start`.
For most apps, like Rails apps, this will work the same way as
`config/appsignal.yml` does now.

If `Appsignal.configure` is called manually in the app, like a Rails
initializer, the `config/appsignal.rb` file is not loaded. Instead, we
recommend moving the `Appsignal.configure` call to the
`config/appsignal.rb` file.

## New behavior

With this change the config is loaded as described in these scenarios:

- Scenario 1:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` is required and configures the gem.
- Scenario 2:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.rb` is required and configures the gem using
    the `Appsignal.configure` helper.
  - The `config/appsignal.yml` is not loaded.
- Scenario 3:
  - `Appsignal.configure` is called.
  - `Appsignal.start` is called.
  - The `config/appsignal.rb` file is not required.
- Scenario 4:
  - `Appsignal.configure` is called.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.yml` file is loaded.
  - `Appsignal.start` is called.
- Scenario 5:
  - `Appsignal.configure` is called.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` file is not loaded.
  - `Appsignal.start` is called.

This is more complex that I would have liked. In hindsight I shouldn't
have made `Appsignal.configure` load the `config/appsignal.yml` file.
That would have made it a bit more predictable. But I don't want
`Appsignal.configure` in the `config/appsignal.rb` file to also load the
`config/appsignal.yml` file, because then it just gets more confusing.

In the next major version I want to remove `config/appsignal.yml`
support, so this is the first step.

## App/root path

I remember some of our users configured the Config class's `root_path`
to specify where to load the config file from.
If an app configures itself via a `config/appsignal.rb`, this makes it
impossible to customize the root path. For that purpose I've added the
`APPSIGNAL_APP_PATH` environment variable to customize the config path.
Which I've also used to make it easier to test the different scenarios.

---

Part of #1324
Add `config/appsignal.rb` config file support. This is a replacement for
the `config/appsignal.yml` config file.

Load the `config/appsignal.rb` file automatically on `Appsignal.start`.
For most apps, like Rails apps, this will work the same way as
`config/appsignal.yml` does now.

If `Appsignal.configure` is called manually in the app, like a Rails
initializer, the `config/appsignal.rb` file is not loaded. Instead, we
recommend moving the `Appsignal.configure` call to the
`config/appsignal.rb` file.

## New behavior

With this change the config is loaded as described in these scenarios:

- Scenario 1:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` is required and configures the gem.
- Scenario 2:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.rb` is required and configures the gem using
    the `Appsignal.configure` helper.
  - The `config/appsignal.yml` is not loaded.
- Scenario 3:
  - `Appsignal.configure` is called.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` file is not loaded.
  - `Appsignal.start` is called.
- Scenario 4:
  - `Appsignal.configure` is called.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.yml` file is loaded.
  - `Appsignal.start` is called.

This is more complex that I would have liked. In hindsight I shouldn't
have made `Appsignal.configure` load the `config/appsignal.yml` file.
That would have made it a bit more predictable. But I don't want
`Appsignal.configure` in the `config/appsignal.rb` file to also load the
`config/appsignal.yml` file, because then it just gets more confusing.

In the next major version I want to remove `config/appsignal.yml`
support, so this is the first step.

## App/root path

I remember some of our users configured the Config class's `root_path`
to specify where to load the config file from.
If an app configures itself via a `config/appsignal.rb`, this makes it
impossible to customize the root path. For that purpose I've added the
`APPSIGNAL_APP_PATH` environment variable to customize the config path.
Which I've also used to make it easier to test the different scenarios.

---

Part of #1324
Add a private method (that has to be publicly accessible within the gem
itself), to load the config. This avoids duplication in the diagnose CLI
when I update it to call this method as well.

Make it more safe by replicating the existing behavior: it will never
return a `nil` object for the Config once started. If it started with an
error or the config file is empty, initialize the config but set
`active` to `false`.
Update the diagnose CLI to call the new `Appsignal._load_config!`
PRIVATE method to initialize the config. This way we have less
duplication of how the config is initialized and we can be sure it works
the same in the diagnose CLI as well.

I've introduced a new PRIVATE `_APPSIGNAL_CONFIG_FILE_ENV` env var to
set the environment, but only in the diagnose CLI context. It's one of
the few ways we can pass the default environment as set by the CLI to
whatever config object gets initialized in the config file.

I didn't use the `APPSIGNAL_APP_ENV` environment variable so the config
option doesn't show up in the report as being set by an env var or
overwrite it when the user has set it.
@backlog-helper

This comment has been minimized.

1 similar comment
@backlog-helper

This comment has been minimized.

@backlog-helper
Copy link


This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

Let's see if I understood the different scenarios right. I rearranged them in a way that worked better for my brain:

  • When Appsignal.configure is called before Appsignal.start, for example, in a Rails initialiser, or in the customer's main.rb:

    • This causes config/appsignal.yml to be loaded, but not config/appsignal.rb.
      • (We could emit a warning here, if both exist, about config/appsignal.rb not being loaded -- suggest that they move the call to Appsignal.configure there)
    • This does not cause AppSignal to be initialised -- Appsignal.start must still be called afterwards.
    • When Appsignal.start is called afterwards, it will not cause any config files to be loaded.
  • When Appsignal.start is called without Appsignal.configure having been called beforehand:

    • This causes config/appsignal.rb to be loaded -- if it is not present, this causes config/appsignal.yml to be loaded.
      • (We could emit a warning here, if both exist, about config/appsignal.yml not being loaded -- suggest that they use only config/appsignal.rb)
      • Within config/appsignal.rb, calls to Appsignal.configure will not cause config/appsignal.yml to be loaded.
    • Outside of config/appsignal.rb, any calls to Appsignal.configure after Appsignal.start has been called will be ignored.

.changesets/add-config-appsignal-rb-file-support.md Outdated Show resolved Hide resolved
lib/appsignal/config.rb Outdated Show resolved Hide resolved
.changesets/add-config-appsignal-rb-file-support.md Outdated Show resolved Hide resolved
These are suggestions from the review and my own improvements.
Add warnings to stderr and the log file when some config combination is
used that may be expected to work, but does not. This is in preparation
of the YAML file removal and to encourage apps to use the
`config/appsignal.rb` config file and/or `Appsignal.configure` helper.
@tombruijn tombruijn merged commit 2623790 into main Nov 11, 2024
127 checks passed
@tombruijn tombruijn deleted the config-rb-file branch November 11, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants