don't continue if a config file is present and invalid

[jjb]

An invalid config file caused problems for us in prod and it was difficult to diagnose.

I can't think of a reason why the gem should support the scenario where a file is present and invalid and it just continues on ignoring the file. I'm guessing the vast majority of users would want things to fail loudly in this scenario. But maybe i'm not aware of other scenarios! 😅

The proposed code in this PR is just made quickly and for discussion - if you're open to this new behavior, give me feedback on how to improve the code, or feel free to close this out and make your own.

Let me know what you think!

[tombruijn]

Hi @jjb! and thanks for the bringing up this point. The philosophy of the AppSignal integrations is that we should never be the one to break/crash your app. We don't reraise the error because that stops the app from starting. We'd prefer to see no data come in to AppSignal, something that can be debugged with our diagnose tool and with the help from support team, rather than bring down an app.

We'd like to know what was difficult to diagnose so we can improve that.

[jjb]

i updated ruby 2.7 -> 3.1, but we are stuck on psych 3.

with appsignal 3.0 it works

for some reason when updating to appsignal higher than that (or maybe we made some other change at the same time, i can't remember the exact combination of events), reading the config file breaks. i think we ignored this in dev because we assumed it was some sort of irrelevant dev message where we don't use appsignal anyway.

when the code went live, our config was ignored. the config does many important things, such as specifying the name of the app.

[jjb]

We'd prefer to see no data come in to AppSignal

i wouldn't mind this behavior in that scenario, but it's not what happens

[tombruijn]

How does reading the config file break for you after upgrading? Can you paste the error for me that is raised in the config file here or at [email protected]? (with a link to this issue)

We've had issues with the psych version recently with installing the extension in issue #904 (which also reads a YAML file), but I haven't seen that happen while reading the config file.

[jjb]

unfortunately i can't remember the combination of things which made our config file unreadable

even if that problem is solved, the larger problem of configuration being complete ignored in production is still an issue

as i said above, you mentioned "no data come in to AppSignal" - that's a good fallback for us, but it's not the current behavior, unless i'm misunderstanding

[tombruijn]

as i said above, you mentioned "no data come in to AppSignal" - that's a good fallback for us, but it's not the current behavior, unless i'm misunderstanding

That's what it should do, unless you have multiple config sources. Then it will use what it can from the initial and env config source to put together a configuration to start AppSignal with.

For example:
If you configure all the required config options through environment variables (app name, Push API key, app env (optional for Rails apps)), but configure the ignore_* config options through the config file, AppSignal will start with the config from the env, but ignore the config from the config file (because it couldn't load it).

[jjb]

gotcha - well the only thing we have in an env variable is our api key. i still don't think that's reasonable behavior "they have a yml file, it can't be read, but they have an api key available, so we'll go ahead and just use all defaults"

[jjb]

here's a related problem

if yaml is breaking, dev environment is silent about it. in the first one, the file is in good condition. in the second one, i introduced yaml syntax errors. this is with 3.3.7

➔ rails c
[2023-03-09T21:26:54 (process) #33925][INFO] appsignal: Not starting, not active for development
Loading development environment (Rails ...)
[development] (main)>

➔ rails c
Loading development environment (Rails ...)
[development] (main)>

[tombruijn]

This is the error I get when I create a syntax error in the YAML in a Rails app with Psych 3.3.4 installed:

$ rails c
appsignal: An error occured while loading the AppSignal config file. Skipping file config.
File: "/app/config/appsignal.yml"
Psych::SyntaxError: (<unknown>): did not find expected key while parsing a block mapping at line 4 column 3

Can you share with me the appsignal.yml file in its broken state so that I can test that? It should print that warning.

Or have you disabled warnings from Ruby apps? Because then too do I not see that warning:

RUBYOPT="-W0" rails c

[jjb]

thanks a lot for the ideas

i will dig into this a bit later and make sure i can reproduce the behavior and get back to you

[jjb]

i disabled spring and bootsnap and turned on class caching and preloading in development.rb

i then made a bunch of errors in my appsignal.yml

interestingly, when i made an erb error, i got a big erb stacktrace

when i removed that error but left other yaml errors, rails console was silent

this is all with appsignal 3.0.27

i also tried the final scenario with 3.3.7, same behavior

no errors

➔ RUBYOPT="-W0" rails c
[2023-03-14T21:33:50 (process) #93803][INFO] appsignal: Not starting, not active for development
Loading development environment (Rails ...)
[development] (main)>

with erb errors

➔ RUBYOPT="-W0" rails c
DeadEnd: Could not find filename from "(erb):8: syntax error, unexpected constant, expecting ')'\n"
...big stack trace continues...

just yaml errors

➔ RUBYOPT="-W0" rails c
Loading development environment (Rails ...)
[development] (main)>

demonstrating yaml errors

[development] (main)> YAML::VERSION
=> "4.0.3"
[development] (main)> YAML.load_file(Rails.root.join('config', 'appsignal.yml'))
Psych::SyntaxError: (/Users/john/myapp/config/appsignal.yml): did not find expected '-' indicator while parsing a block collection at line 12 column 5
from /Users/john/.rbenv/versions/3.1.2/lib/ruby/3.1.0/psych.rb:455:in `parse'

➔ env |grep -i appsi
➔ grep -i appsi .env*
➔

[jjb]

I just emailed you my before and after appsignal.yml files.

Let me know if you want me to try anything else, thanks! :)

[tombruijn]

Hi @jjb, I've discussed it with the team. We'll change the configuration loading to not start AppSignal when the configuration file fails to load. I've created a new issue for it #926. We don't know when we'll pick up this change just yet.

Are you able to continue using AppSignal in your app?
I have not been able to reproduce the issue of not seeing the YAML or ERB errors in the logs. If I start an app with the broken config file you sent, I see the error in the logs. Just make sure not to use the RUBYOPT="-W0", as that hides the warning.

Closing this PR.

[jjb]

Just make sure not to use the RUBYOPT="-W0", as that hides the warning.

🤦 whoops, i misread your original note about that 😄

but, same behavior:

➔ RUBYOPT="-W1" rails c
Loading development environment (Rails ...)

with -W2 there is a giant stream of things about my code

my development.rb had log level set to warn, i set it to debug, got more log lines, but nothing about appsignal. also tried disabling lograge, same behavior.

🤷

[tombruijn]

That's really weird behavior. I am unable to reproduce still that I don't see the error being logged upon parsing the appsignal.yml file. I don't know why it would not show the error if an error occurs while parsing. Maybe another gem is interfering? You could always send us your Gemfile and we'll look for some usual suspects.

[jjb]

it's spring!

spring (4.1.1)
appsignal (3.3.7)

CleanShot.2023-03-17.at.00.49.01.mp4

[jjb]

commenting out this fallback results in an error being raised every time, killing spring is not needed (and of course intended behavior, of rails continuing on, does not work)

@file_config = load_from_disk# || {}

if i remove the above comment, and add this line below, it is only printed on first invocation

      initializer "appsignal.configure_rails_initialization" do |app|
        puts "railtie initializer block" # <-----
        Appsignal::Integrations::Railtie.initialize_appsignal(app)
      end

so i think there is something going on with the railtie implementation. i tried looking into it but didn't find anything. maybe there is something to copy here:

• https://github.com/bensheldon/good_job/blob/main/lib/good_job/engine.rb
• https://github.com/sidekiq/sidekiq/blob/main/lib/sidekiq/rails.rb

although those only have additional things wrapping the code inside the initializer block, which appsignal code would already never get to.

[jjb]

another note, probably not related to the mystery behavior, but something you may also want to improve: when Appsignal::Config.new raises an error in initialize_appsignal, we never get to Appsignal.start_logger, so log lines (such as in Appsignal.start) after that are not shown

actually i was wrong, it does continue on with the fallback hash. so i don't know why logging isn't working in my experiments

here i see my debug numbers printed, but nothing from logger

[jjb]

I think i was overthinking this before - spring isn't reloading the file because it already successfully loaded, it's that simple 😅 so on the second run, it doesn't try to load it again

[tombruijn]

Ah spring strikes again. Good to know what caused the strange behavior.