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

Don't activate AppSignal when the configuration file fails to load #926

Closed
2 tasks done
tombruijn opened this issue Mar 15, 2023 · 4 comments · Fixed by #995
Closed
2 tasks done

Don't activate AppSignal when the configuration file fails to load #926

tombruijn opened this issue Mar 15, 2023 · 4 comments · Fixed by #995
Assignees
@tombruijn
Labels
enhancement

Comments

@tombruijn
Copy link
Member

tombruijn commented Mar 15, 2023 •
edited
Loading

When the configuration file config/appsignal.yml fails to load (a YAML or ERB error occurs while parsing), do not start AppSignal. Even if it has a valid config based on configuration from other sources (like the initial config and environment variables).

Discussed in:

To do

  • Find a good way to deactivate AppSignal when an error was encountered during the config file (config/appsignal.yml) loading and parsing. For example: Use the override config source and add active: false to it?
  • If necessary, mark this error in the diagnose report sent to the server, or display it in the diagnose viewer.

This is a breaking change in how the config is loaded. Communicate this accordingly.
@tombruijn tombruijn mentioned this issue Mar 15, 2023
Closed
@jjb
Copy link
Contributor

jjb commented Mar 15, 2023 •
edited
Loading

Thank you!

idea:

version 3

  • new option, something like --fail-on-config-error flag, which activates this behavior
  • if user is not using this flag but there is a config error, print message informing that the flag exists

version 4

  • if --fail-on-config-error is used, print message informing that it is now default

@tombruijn
Copy link
Member Author

I like the option/flag to activate the behavior opt-in and make it the default in the next major release. We can't add a CLI option/flag to whatever process starts AppSignal, but we could make it an environment variable like APPSIGNAL_ON_CONFIG_ERROR=fail. It wouldn't be a config option for the config file, because this is about the config file itself failing to be parsed.

@jjb
Copy link
Contributor

jjb commented Aug 22, 2023

sounds good

FWIW, in my use case, i am happy that I now have tests guarding me against this scenario. so, no need from me at least to build the transitional feature in version 3 as i suggested above.

tombruijn added a commit that referenced this issue Aug 23, 2023
@tombruijn
Add behavior to not start on config file error
681503d 
When the `config/appsignal.yml` file triggers an error while parsing it,
we ignore the config file source. This can cause some unexpected
behavior, like missing config and reporting it as the wrong app name.
More importantly, any filter config options in the config file are
missing and we may send PII data that's supposed to be filtered out.

We don't want to change the behavior now, as it's a breaking change. Add
a flag to opt-in to this new behavior, using the
`APPSIGNAL_INACTIVE_ON_CONFIG_FILE_ERROR` config option. Then for the
next major version we should make this the new default behavior.

If accepted, I'll make this more visible in the diagnose report.

Closes #926
@tombruijn tombruijn self-assigned this Aug 23, 2023
tombruijn added a commit that referenced this issue Aug 23, 2023
@tombruijn
Add behavior to not start on config file error
bb34e25 
When the `config/appsignal.yml` file triggers an error while parsing it,
we ignore the config file source. This can cause some unexpected
behavior, like missing config and reporting it as the wrong app name.
More importantly, any filter config options in the config file are
missing and we may send PII data that's supposed to be filtered out.

We don't want to change the behavior now, as it's a breaking change. Add
a flag to opt-in to this new behavior, using the
`APPSIGNAL_INACTIVE_ON_CONFIG_FILE_ERROR` config option. Then for the
next major version we should make this the new default behavior.

If accepted, I'll make this more visible in the diagnose report.

Closes #926
@tombruijn tombruijn mentioned this issue Aug 23, 2023
Merged
@tombruijn
Copy link
Member Author

The change in #991 was released in package 3.4.12. You can use the APPSIGNAL_INACTIVE_ON_CONFIG_FILE_ERROR=1 variable to opt-in to this behavior.

@tombruijn tombruijn reopened this Aug 30, 2023
tombruijn added a commit that referenced this issue Sep 4, 2023
@tombruijn
Add configuration modifiers to Ruby diagnose
4f36ff7 
Since PR #991 we have a
configuration load modifier that does not active the Ruby gem when this
modifier is set.
Update the report to include this modifier for better debugging.
This will also require a server change to show this modifier section.

Closes #926
This was referenced Sep 4, 2023
Merged
Merged
tombruijn added a commit that referenced this issue Sep 11, 2023
@tombruijn
Add configuration modifiers to Ruby diagnose (#995)
6932bb3 
Since PR #991 we have a
configuration load modifier that does not active the Ruby gem when this
modifier is set.
Update the report to include this modifier for better debugging.
This will also require a server change to show this modifier section.

Closes #926
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tombruijn tombruijn

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add configuration modifiers to Ruby diagnose appsignal/appsignal-ruby
2 participants
@jjb @tombruijn