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 support for tags #45

Closed

Conversation

nachomdo
Copy link

This PR is intended to extend the functionality of the NoticeReporter interface to support tags.

image

@sgerrand
Copy link
Contributor

👋 @joshuap @starrhorne: Would you please review these changes. We'd like to use more of the features available in Honeybadger's API via the Java client.

@joshuap
Copy link
Member

joshuap commented May 24, 2018

Sorry for the delay. I think this is looking good. A few things:

The build is failing on Travis because (I think) the latest container image doesn't support JDK 7. You should be able to get oraclejdk7 and oraclejdk8 builds passing by pulling in this commit: 0ffc680

The openjdk7 build will still fail, however, and I'm not sure why. Any ideas? See here:

https://travis-ci.org/honeybadger-io/honeybadger-java/builds/383374481

Beyond that, could you update this PR to add a usage example to README.md, @nachomdo? Thanks!

Also looping in @dekobon and his folks in case they have any comments on this.

@dekobon
Copy link
Contributor

dekobon commented May 25, 2018

For what it is worth, the honeybadger-io 2.0 work that I was in the middle of supported tags: https://github.com/dekobon/honeybadger-java/tree/2.0.0

I had my work situation suddenly change and I wasn't able to merge the work. I believe it is done. However, I have a concern about it not reporting the environment correctly all of the time based on a sample application that I was working on.

I believe this is a more robust set of changes that moves Honeybadger forward. If anyone would like to adopt it and move it the last 5% forward, it would be much appreciated.

@nachomdo
Copy link
Author

@joshuap Thanks for reviewing these changes. Regarding the issues in TravisCI, I added your commit to run the build in the old precise environment but for whatever reason is running without the environment vars set in TravisCI. However, your build (https://travis-ci.org/honeybadger-io/honeybadger-java/builds/383374481) run with all the environment variables properly set. I don't know if it could be an issue with the settings in TravisCI.
Also fixed the buffer overflow on openjdk 7 applying this workaround - travis-ci/travis-ci#5227

Please check the lack of configuration in the link below:
https://travis-ci.org/honeybadger-io/honeybadger-java/jobs/383617642#L273

@joshuap
Copy link
Member

joshuap commented May 29, 2018

@nachomdo thanks! I think that config is something we set in the Travis UI. We should probably get the encrypted value added to our .travis.yml.

Heads up: I am currently working with @dekobon to get his 2.0.0 branch cleaned up and ready for release. It includes a bunch of improvements to the client, and also supports tags. Will check back here when we get closer to a release.

@JasonTrue
Copy link
Collaborator

@nachomdo the 2.0.0 branch should be a bit closer to release-ready. If you've got a moment can you grab it and see if the tags support there works for you?

I may put together a solution to allow hooks in the existing error handlers to avoid the need for a custom implementation (instead of manually invoking reportError) but haven't quite gotten there yet.

@joshuap
Copy link
Member

joshuap commented Jul 10, 2018

I'm going to close this PR--let's continue discussion of the new tags support on #56.

@joshuap joshuap closed this Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants