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

Capture event for HTTP requests resulted in server error #2287

Merged
merged 43 commits into from Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
4adc744
test
marandaneto Oct 5, 2022
371db9a
HTTP client errors for OkHttp
marandaneto Oct 10, 2022
090a7df
fix
marandaneto Oct 10, 2022
20f8b09
Format code
getsentry-bot Oct 10, 2022
76c4567
fix
marandaneto Oct 10, 2022
a664017
Merge branch 'main' into feat/httpclienterrors
marandaneto Oct 10, 2022
d046806
Merge branch 'feat/httpclienterrors' of https://github.com/getsentry/…
marandaneto Oct 10, 2022
54cac05
implement review
marandaneto Oct 12, 2022
3804a5c
Format code
getsentry-bot Oct 12, 2022
d970ff9
move the propagation targets to utils
marandaneto Oct 12, 2022
2c113a9
Merge branch 'feat/httpclienterrors' of https://github.com/getsentry/…
marandaneto Oct 12, 2022
bee4510
Format code
getsentry-bot Oct 12, 2022
1541330
ref
marandaneto Oct 12, 2022
cf81c16
merge
marandaneto Oct 12, 2022
412bc0e
remove tags
marandaneto Oct 12, 2022
9136b37
fix
marandaneto Oct 12, 2022
2284f88
fix api
marandaneto Oct 12, 2022
6268a19
fix build
marandaneto Oct 12, 2022
566d70e
Format code
getsentry-bot Oct 12, 2022
9b5e6bb
Merge branch 'main' into feat/httpclienterrors
marandaneto Oct 13, 2022
176e7ef
fix
marandaneto Oct 14, 2022
e896271
merge
marandaneto Oct 14, 2022
67fc1c1
fix conflict
marandaneto Oct 14, 2022
0e27884
add body size to request field
marandaneto Oct 14, 2022
a6211b8
api
marandaneto Oct 14, 2022
2f10f79
fix naming
marandaneto Oct 14, 2022
a26cde4
add tests
marandaneto Oct 14, 2022
d452d9d
add changelog
marandaneto Oct 14, 2022
d27d2c2
review
marandaneto Oct 17, 2022
e3bfc1f
Merge branch 'main' into feat/httpclienterrors
marandaneto Oct 17, 2022
c06174f
Format code
getsentry-bot Oct 17, 2022
76168e0
fix
marandaneto Oct 17, 2022
3fd6b01
Merge branch 'feat/httpclienterrors' of https://github.com/getsentry/…
marandaneto Oct 17, 2022
7572c8a
tests
marandaneto Oct 18, 2022
68408b5
fix test
marandaneto Oct 18, 2022
99b62a3
Merge branch 'main' into feat/httpclienterrors
marandaneto Oct 18, 2022
f0a6294
fix todos
marandaneto Oct 18, 2022
a3ca145
fixm essage
marandaneto Oct 19, 2022
38aeba2
Merge branch 'main' into feat/httpclienterrors
marandaneto Oct 19, 2022
6a4013b
review
marandaneto Oct 20, 2022
57d3776
fix
marandaneto Oct 20, 2022
636ed7e
Merge branch 'main' into feat/httpclienterrors
marandaneto Oct 20, 2022
b79a165
review
marandaneto Oct 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -78,6 +78,8 @@ final class ManifestMetadataReader {
static final String CLIENT_REPORTS_ENABLE = "io.sentry.send-client-reports";
static final String COLLECT_ADDITIONAL_CONTEXT = "io.sentry.additional-context";

static final String SEND_DEFAULT_PII = "io.sentry.send-default-pii";

/** ManifestMetadataReader ctor */
private ManifestMetadataReader() {}

Expand Down Expand Up @@ -297,6 +299,9 @@ static void applyMetadata(
sdkInfo.setName(readStringNotNull(metadata, logger, SDK_NAME, sdkInfo.getName()));
sdkInfo.setVersion(readStringNotNull(metadata, logger, SDK_VERSION, sdkInfo.getVersion()));
options.setSdkVersion(sdkInfo);

options.setSendDefaultPii(
readBool(metadata, logger, SEND_DEFAULT_PII, options.isSendDefaultPii()));
}

options
Expand Down
1 change: 1 addition & 0 deletions sentry-android-okhttp/build.gradle.kts
Expand Up @@ -71,6 +71,7 @@ dependencies {

compileOnly(Config.Libs.okhttpBom)
compileOnly(Config.Libs.okhttp)
// compileOnly(Config.CompileOnly.jetbrainsAnnotations)

implementation(kotlin(Config.kotlinStdLib, KotlinCompilerVersion.VERSION))

Expand Down
@@ -0,0 +1,4 @@
package io.sentry.android.okhttp

// @ApiStatus.Internal
class SentryHttpClientError(override val message: String) : RuntimeException(message)
Expand Up @@ -6,18 +6,27 @@ import io.sentry.Hint
import io.sentry.HubAdapter
import io.sentry.IHub
import io.sentry.ISpan
import io.sentry.SentryEvent
import io.sentry.SpanStatus
import io.sentry.TracePropagationTargets
import io.sentry.TypeCheckHint.OKHTTP_REQUEST
import io.sentry.TypeCheckHint.OKHTTP_RESPONSE
import io.sentry.exception.ExceptionMechanismException
import io.sentry.protocol.Mechanism
import io.sentry.protocol.Message
import okhttp3.Headers
import okhttp3.Interceptor
import okhttp3.Request
import okhttp3.Response
import java.io.IOException

class SentryOkHttpInterceptor(
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
private val hub: IHub = HubAdapter.getInstance(),
private val beforeSpan: BeforeSpanCallback? = null
private val beforeSpan: BeforeSpanCallback? = null,
// TODO: should this be under the options or here? also define the names
private val captureFailedRequests: Boolean = false,
private val failedRequestStatusCode: List<StatusCodeRange> = listOf(StatusCodeRange(500, 599)),
private val failedRequestsTargets: List<String> = listOf(".*")
) : Interceptor {

constructor(hub: IHub) : this(hub, null)
Expand Down Expand Up @@ -53,6 +62,10 @@ class SentryOkHttpInterceptor(
response = chain.proceed(request)
code = response.code
span?.status = SpanStatus.fromHttpStatusCode(code)

// OkHttp errors (4xx, 5xx) don't throw, so it's safe to call within this block.
captureEvent(request, response)

return response
} catch (e: IOException) {
span?.apply {
Expand Down Expand Up @@ -104,6 +117,123 @@ class SentryOkHttpInterceptor(
}
}

// TODO: ignore exceptions of the type UnknownHostException

private fun captureEvent(request: Request, response: Response) {
// not possible to get a parameterized url, but we remove at least the
// query string and the fragment.
// url example: https://api.github.com/users/getsentry/repos/#fragment?query=query
// url will be: https://api.github.com/users/getsentry/repos/
// ideally we'd like a parameterized url: https://api.github.com/users/{user}/repos/
// but that's not possible
var requestUrl = request.url.toString()

val query = request.url.query
if (!query.isNullOrEmpty()) {
requestUrl = requestUrl.replace("?$query", "")
}

val urlFragment = request.url.fragment
if (!urlFragment.isNullOrEmpty()) {
requestUrl = requestUrl.replace("#$urlFragment", "")
}

if (!captureFailedRequests || !TracePropagationTargets.contain(failedRequestsTargets, requestUrl) || !containsStatusCode(response.code)) {
return
}

val mechanism = Mechanism().apply {
type = "SentryOkHttpInterceptor"

// // with description, mechanism in the UI is buggy
// description = message

// TODO: should it be synthetic? likely not, mechanism SentryOkHttpInterceptor should be considered for grouping
// synthetic = true
}
val exception = SentryHttpClientError("Event was captured because the request status code was ${response.code}")
// TODO: okhttp thread uses thread pool so its always the same stack trace (okhttp frames only), stacktrace is set as snapshot=true, is that ok?
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
val mechanismException = ExceptionMechanismException(mechanism, exception, Thread.currentThread(), true)
val event = SentryEvent(mechanismException)

// TODO: Do we need a message if the request and response already have the info?
val sentryMessage = Message().apply {
message = "HTTP url: %s status: %s"
params = listOf(requestUrl, response.code.toString())
}
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

val hint = Hint()
hint.set("request", request)
hint.set("response", response)

// TODO: remove after fields indexed
val tags = mutableMapOf<String, String>()
tags["status_code"] = response.code.toString()
tags["url"] = requestUrl
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

val unknownRequestFields = mutableMapOf<String, Any>()

val sentryRequest = io.sentry.protocol.Request().apply {
url = requestUrl
cookies = request.headers["Cookie"]
method = request.method
queryString = query
headers = getHeaders(request.headers)
fragment = urlFragment

request.body?.contentLength().ifHasValidLength {
// TODO: should be mapped in relay and added to the protocol
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
unknownRequestFields["body_size"] = it
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
}

unknown = unknownRequestFields.ifEmpty { null }
}

val sentryResponse = io.sentry.protocol.Response().apply {
cookies = response.headers["Cookie"]
headers = getHeaders(response.headers)
statusCode = response.code

response.body?.contentLength().ifHasValidLength {
bodySize = it
}
}

event.tags = tags
event.request = sentryRequest
event.contexts.setResponse(sentryResponse)
event.message = sentryMessage

hub.captureEvent(event, hint)
}

private fun containsStatusCode(statusCode: Int): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could be called something like shouldStatusCodeCauseSentryEvent or similar?

Also should it be extracted into some util so it can be reused from other integrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, isInRange already can be reused.
In case the language has a built-in type for Range instead of HttpStatusCodeRange, they will need to implement this anyway.
Java does not have Range obj as part of the language.

for (item in failedRequestStatusCode) {
if (item.isInRange(statusCode)) {
return true
}
}
return false
}

private fun getHeaders(requestHeaders: Headers): MutableMap<String, String>? {
// TODO: should it be under isSendDefaultPii?
// Server already does data scrubbing
if (!hub.options.isSendDefaultPii) {
return null
}
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

val headers = mutableMapOf<String, String>()

for (i in 0 until requestHeaders.size) {
// TODO: should we remove sentry-trace and baggage from headers?
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
val name = requestHeaders.name(i)
val value = requestHeaders.value(i)
headers[name] = value
}
return headers
}

/**
* The BeforeSpan callback
*/
Expand Down
@@ -0,0 +1,12 @@
package io.sentry.android.okhttp

// import org.jetbrains.annotations.ApiStatus

// @ApiStatus.Internal
class StatusCodeRange(private val min: Int, private val max: Int) {
constructor(statusCode: Int) : this(statusCode, statusCode)

fun isInRange(statusCode: Int): Boolean {
return statusCode in min..max
}
}
Expand Up @@ -135,5 +135,8 @@
<!-- how to enable the attach screenshot feature-->
<meta-data android:name="io.sentry.attach-screenshot" android:value="true" />

<!-- how to enable the send default pii-->
<meta-data android:name="io.sentry.send-default-pii" android:value="true" />

</application>
</manifest>
Expand Up @@ -10,7 +10,8 @@ interface GitHubService {
@GET("users/{user}/repos")
fun listRepos(@Path("user") user: String): Call<List<Repo>>

@GET("users/{user}/repos")
// TODO: @GET("users/{user}/repos/#test") throws 404
@GET("users/{user}/repos/")
suspend fun listReposAsync(@Path("user") user: String, @Query("per_page") perPage: Int): List<Repo>
}

Expand Down
@@ -1,13 +1,22 @@
package io.sentry.samples.android

import io.sentry.android.okhttp.SentryOkHttpInterceptor
import io.sentry.android.okhttp.StatusCodeRange
import okhttp3.OkHttpClient
import retrofit2.Retrofit
import retrofit2.converter.gson.GsonConverterFactory

object GithubAPI {

private val client = OkHttpClient.Builder().addInterceptor(SentryOkHttpInterceptor()).build()
private val client = OkHttpClient.Builder().addInterceptor(
SentryOkHttpInterceptor(
captureFailedRequests = true,
// TODO: 200 just for testing
failedRequestStatusCode = listOf(
StatusCodeRange(200, 599)
)
)
).build()

private val retrofit = Retrofit.Builder()
.baseUrl("https://api.github.com/")
Expand Down
Expand Up @@ -90,7 +90,12 @@ fun Github(
val scope = rememberCoroutineScope()

LaunchedEffect(perPage) {
result = GithubAPI.service.listReposAsync(user.text, perPage).random().full_name
try {
result = GithubAPI.service.listReposAsync(user.text, perPage).random().full_name
} catch (e: Throwable) {
// TODO: event processor that converts retrofit HttpException to a proper sentry event
Sentry.captureException(e)
}
}

Column(
Expand All @@ -108,7 +113,12 @@ fun Github(
Button(
onClick = {
scope.launch {
result = GithubAPI.service.listReposAsync(user.text, perPage).random().full_name
try {
result =
GithubAPI.service.listReposAsync(user.text, perPage).random().full_name
} catch (e: Throwable) {
Sentry.captureException(e)
}
}
},
modifier = Modifier.padding(top = 32.dp)
Expand Down
2 changes: 2 additions & 0 deletions sentry/src/main/java/io/sentry/TracePropagationTargets.java
Expand Up @@ -4,6 +4,8 @@
import java.util.List;
import org.jetbrains.annotations.NotNull;

// TODO: rename class to something more generic

/**
* Checks if an URL matches the list of origins to which `sentry-trace` header should be sent in
* HTTP integrations.
Expand Down
33 changes: 23 additions & 10 deletions sentry/src/main/java/io/sentry/protocol/Contexts.java
Expand Up @@ -22,9 +22,9 @@ public final class Contexts extends ConcurrentHashMap<String, Object> implements
public Contexts() {}

public Contexts(final @NotNull Contexts contexts) {
for (Map.Entry<String, Object> entry : contexts.entrySet()) {
for (final Map.Entry<String, Object> entry : contexts.entrySet()) {
if (entry != null) {
Object value = entry.getValue();
final Object value = entry.getValue();
if (App.TYPE.equals(entry.getKey()) && value instanceof App) {
this.setApp(new App((App) value));
} else if (Browser.TYPE.equals(entry.getKey()) && value instanceof Browser) {
Expand All @@ -40,6 +40,8 @@ public Contexts(final @NotNull Contexts contexts) {
this.setGpu(new Gpu((Gpu) value));
} else if (SpanContext.TYPE.equals(entry.getKey()) && value instanceof SpanContext) {
this.setTrace(new SpanContext((SpanContext) value));
} else if (Response.TYPE.equals(entry.getKey()) && value instanceof Response) {
this.setResponse(new Response((Response) value));
} else {
this.put(entry.getKey(), value);
}
Expand Down Expand Up @@ -109,17 +111,25 @@ public void setGpu(final @NotNull Gpu gpu) {
this.put(Gpu.TYPE, gpu);
}

public @Nullable Response getResponse() {
return toContextType(Response.TYPE, Response.class);
}

public void setResponse(final @NotNull Response response) {
this.put(Response.TYPE, response);
}

// region json

@Override
public void serialize(@NotNull JsonObjectWriter writer, @NotNull ILogger logger)
public void serialize(final @NotNull JsonObjectWriter writer, final @NotNull ILogger logger)
throws IOException {
writer.beginObject();
// Serialize in alphabetical order to keep determinism.
List<String> sortedKeys = Collections.list(keys());
java.util.Collections.sort(sortedKeys);
for (String key : sortedKeys) {
Object value = get(key);
final List<String> sortedKeys = Collections.list(keys());
Collections.sort(sortedKeys);
for (final String key : sortedKeys) {
final Object value = get(key);
if (value != null) {
writer.name(key).value(logger, value);
}
Expand All @@ -130,9 +140,9 @@ public void serialize(@NotNull JsonObjectWriter writer, @NotNull ILogger logger)
public static final class Deserializer implements JsonDeserializer<Contexts> {

@Override
public @NotNull Contexts deserialize(@NotNull JsonObjectReader reader, @NotNull ILogger logger)
throws Exception {
Contexts contexts = new Contexts();
public @NotNull Contexts deserialize(
final @NotNull JsonObjectReader reader, final @NotNull ILogger logger) throws Exception {
final Contexts contexts = new Contexts();
reader.beginObject();
while (reader.peek() == JsonToken.NAME) {
final String nextName = reader.nextName();
Expand All @@ -159,6 +169,9 @@ public static final class Deserializer implements JsonDeserializer<Contexts> {
case SpanContext.TYPE:
contexts.setTrace(new SpanContext.Deserializer().deserialize(reader, logger));
break;
case Response.TYPE:
contexts.setResponse(new Response.Deserializer().deserialize(reader, logger));
break;
default:
Object object = reader.nextObjectOrNull();
if (object != null) {
Expand Down