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

If inside a HTTP request, include URL in sentry error #775

Merged
merged 10 commits into from
Nov 26, 2015
Merged

Conversation

tpetr
Copy link
Contributor

@tpetr tpetr commented Nov 23, 2015

Will update PR with full implementation soon -- wanted to get @jhaber's eyes on the request stash stuff.


RequestStash() {
this.url = new ThreadLocal<>();
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: ditch the constructor and initialize inline

@jhaber
Copy link
Member

jhaber commented Nov 23, 2015

lgtm, afaik Guice doesn't have anything to make this easier. The other option would be to have a provider that catches the OutOfScopeException, ie, something like this:

  @Provides
  @Named("request.path")
  public Optional<String> providesRequestPath(Provider<ContainerRequest> request) {
    try {
      return Optional.of(request.get().getPath());
    } catch (OutOfScopeException e) {
      return Optional.absent();
    }
  }

This is simpler in some ways, but the drawback is you need to be very careful to always inject a Provider<Optional<String>> when using it (if you accidentally inject Optional<String> into a singleton without wrapping in a provider, it will cache an empty value forever and be hard to track down)

@tpetr
Copy link
Contributor Author

tpetr commented Nov 24, 2015

@jhaber gotcha, thanks. I originally hit issues with your OOSE-catching approach in a standalone provider, but I think I just made a mistake on my end because it's working now. I'm not too worried about the only using Provider<> caveat because we're only using the provider in one location.

@jhaber
Copy link
Member

jhaber commented Nov 24, 2015

cool lgtm

@tpetr tpetr added this to the 0.4.6 milestone Nov 24, 2015
@tpetr tpetr added the hs_qa label Nov 24, 2015
tpetr pushed a commit that referenced this pull request Nov 26, 2015
If inside a HTTP request, include URL in sentry error
@tpetr tpetr merged commit b834172 into master Nov 26, 2015
@tpetr tpetr deleted the sentry-url branch November 26, 2015 04:56
@tpetr tpetr removed hs_qa labels Nov 26, 2015
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.

2 participants