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

[WIP] fix honeybadger config #50

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmartin-sul
Copy link
Member

@jmartin-sul jmartin-sul commented Dec 16, 2016

  • the overall idea is that honeybadger settings will be obtained from Application.xml properties, as is done with stacks timeout and such.
    • as such, the honeybadger API key and env name properties will have to be added to the Application.xml definition in puppet. we should remove the equivalent environment variables, since we won't be using those.
  • the switch in approach is because it turns out it's a pain to read environment variables in the wowza plugin. you can't just do System.getenv(): i suspect this is a JVM security manager restriction or something, but you have to write another wowza module that implements IServerNotify and gets compiled into a JAR and enabled in Server.xml. that seemed like a lot of new configuration and testing when we can already put private custom settings in Application.xml. the only reason for using environment variables initially was that the Honeybadger docs made it seem like that'd be the easiest way for its config machinery to pick up settings (see https://github.com/honeybadger-io/honeybadger-java#advanced-configuration).
  • this works fine in the unit tests, but since the key for my dev instance of wowza has expired, i can't actually test it. forgetting to test this with the actual wowza server (and relying on the unit tests) is what led to this unexpectedly breaking upon deployment last time, so this should definitely be tested for real before it's actually tagged and built and deployed for real.
  • you'll notice some use of doReturn(...).when(spyModule).method(), instead of the usual when(spyModule.method()).thenReturn(...). this is because by default, the latter approach will call out to the actual method on the spyModule instance, whereas the doReturn approach will just return the mock result w/o calling the underlying method. in the cases where doReturn was used, calling out to the actual method caused errors, and was running code that wasn't the point of the test in question anyway.

so, if this looks like a reasonable approach, i think this is the TODO list:

  • tweak whatever's unsatisfactory with this PR in terms of how the code reads.
  • test w/ an actual running wowza server and make sure that it picks up the settings from Application.xml, or logs the right errors if they aren't present.
  • add the Application.xml settings in a puppet PR. (on prod only, since we probably don't want alerts for the staging environment)
  • update the conf/version file, tag the new version.
  • build the new version in jenkins.
  • puppet PR to specify the new version of the plugin for deployment.
  • add the Honeybadger Java API JAR file to Puppet, so it can get deployed to Wowza's lib/ directory together with the dlss-wowza jar file (on prod only, since we probably don't want alerts for the staging environment) - honeybadger-java-1.1.0.jar

since we're so close to winter break, i'd propose holding off on the actual deployment even if this all looks fine as-is and has no problems with testing, though i suspect there will be feedback on this and a possible lack of time to deploy and test anyway. still, it'd be nice to get feedback on what's here.

sorry that this seemingly simple error reporting implementation has become a bit of a quagmire!

… come from Application.xml) for honeybadger config

* SulWowza.java: move registerUncaughtExceptionHandler() and initNoticeReporter() into initHoneybadger.  initHoneybadger takes IApplicationInstance instance so it can get application properties.
* TestSulWowza.java: adjust mocking for switch to application properties from env vars.
* Application.xml: example honeybadger config properties
* TestParsingFromRequestInfo.java, TestValidationMethods.java, TestVerifyStacksToken.java: remove unnecessary test setup.
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 95.909% when pulling 474491ed9d415db3a2ce7f1f9fa54c002e805171 on 46-fix-honeybadger-config into 3ad1703 on master.

@jmartin-sul jmartin-sul force-pushed the 46-fix-honeybadger-config branch from 474491e to 51c9a18 Compare December 16, 2016 13:04
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 95.909% when pulling 51c9a18 on 46-fix-honeybadger-config into 3ad1703 on master.

@hannahfrost
Copy link
Member

maybe 2 days

@tingulfsen
Copy link
Contributor

I think this PR is good to go, having discussed it with @ndushay - the TODO list above looks complete, so it should just be a question of following that recipe.

@tingulfsen
Copy link
Contributor

tingulfsen commented Nov 7, 2018

Testing procedure:

  • Check out this branch, and build and test:
    ./gradlew clean && ./gradlew build && ./gradlew check && ./gradlew test
  • The new JAR file should be built in ./build/libs/, use scp to copy it over to the stage server.
  • On the stage server:
    • Ensure that the JAR file you built above is copied into the lib/ directory of the most recent Wowza version (currently this would be /opt/app/wowza/WowzaStreamingEngine-4.7.6/lib/. I think you should copy that new JAR file to be named dlss-wowza-v0.4.4.jar AFTER taking a backup of the old version JAR file first.
    • Ensure that honeybadger-java-1.1.0.jar is also installed in the same lib/ directory, as explained above.
    • Edit /opt/app/wowza/WowzaStreamingEngine-4.7.6/conf/stacks/Application.xml to add HONEYBADGER_API_KEY and ENV properties as shown in this PR. The API key can be obtained from within the Honeybadger page for the DLSS-Wowza project.
  • Follow the directions here to log into the Wowza Streaming Engine Manager: https://github.com/sul-dlss/DevOpsDocs/blob/master/projects/media/wowza/operations_concerns.md#wowza-admin-enginemanager then click the Server tab, and restart the server using the button in the upper right corner.
  • Now check the log files to confirm the server restarted OK, and you should be ready to stream.

An easy way to test that Honeybadger errors are reported would be to "sabotage" the Application.xml file to e.g. change the Stacks callback URL to start with hts instead of https - this should create a MalformedURLException, which should be sent to Honeybadger.

@tingulfsen
Copy link
Contributor

Note: I've found that uncaught exceptions don't seem to get reported to Honeybadger, despite the code in void registerUncaughtExceptionHandler(). Having investigated this a little bit, it seems to me that our code should be correct (I've been reading the source code at https://github.com/honeybadger-io/honeybadger-java), and I don't understand what the problem is.

However, this PR still represent a great improvement over what we have now, so I think it should be merged and deployed to production per the steps above.

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.

4 participants