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

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

merged 10 commits into from Nov 26, 2015

Conversation

@tpetr
Copy link
Member

@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<>();
}

This comment has been minimized.

@jhaber

jhaber Nov 23, 2015
Member

nit: ditch the constructor and initialize inline

@jhaber
Copy link
Member

@jhaber 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
Member Author

@tpetr 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 jhaber commented Nov 24, 2015

cool lgtm

@tpetr tpetr added the hs_staging label Nov 24, 2015
@tpetr tpetr added this to the 0.4.6 milestone Nov 24, 2015
@tpetr tpetr added the hs_qa label Nov 24, 2015
@tpetr tpetr added the hs_stable label Nov 25, 2015
tpetr added 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
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@tpetr tpetr deleted the sentry-url branch Nov 26, 2015
@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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.