Skip to content

health dashboards: remove path templatization logic #9

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

Merged
merged 8 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,4 @@ internal object CaptureJniLibrary {
* directly as it allows for centralized control over error flood controls.
*/
external fun reportError(message: String, stackTraceProvider: StackTraceProvider)

/**
* Normalizes a URL path, replacing any high cardinality path substrings with "<id>".
*/
external fun normalizeUrlPath(urlPath: String): String?
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

package io.bitdrift.capture.network

import io.bitdrift.capture.CaptureJniLibrary
import io.bitdrift.capture.InternalFieldsMap
import io.bitdrift.capture.events.span.SpanField
import io.bitdrift.capture.providers.FieldValue
Expand Down Expand Up @@ -36,9 +35,6 @@ data class HttpRequestInfo @JvmOverloads constructor(
) {
internal val name: String = "HTTPRequest"

// Lazy since the creation of the `HTTPRequestInfo` can happen before the logger is initialized
// and trying to call a native `CaptureJniLibrary.normalizeUrlPath` method before the `Capture`
// library is loaded leads to unsatisfied linking error.
internal val fields: InternalFieldsMap by lazy {
// Do not put body bytes count as a common field since response log has a more accurate
// measurement of request' body count anyway.
Expand All @@ -49,7 +45,7 @@ data class HttpRequestInfo @JvmOverloads constructor(
}

internal val commonFields: InternalFieldsMap by lazy {
val fields = buildMap {
extraFields.toFields() + buildMap {
put(SpanField.Key.ID, FieldValue.StringField(spanId.toString()))
put(SpanField.Key.NAME, FieldValue.StringField("_http"))
put(SpanField.Key.TYPE, FieldValue.StringField(SpanField.Value.TYPE_START))
Expand All @@ -58,14 +54,11 @@ data class HttpRequestInfo @JvmOverloads constructor(
putOptional(HttpFieldKey.PATH, path?.value)
putOptional(HttpFieldKey.QUERY, query)
path?.let {
@Suppress("SwallowedException")
val normalized: String? = it.template?.let { it }
?: try { CaptureJniLibrary.normalizeUrlPath(it.value) } catch (e: Throwable) { null }
putOptional(HttpFieldKey.PATH_TEMPLATE, normalized)
it.template?.let {
put(HttpFieldKey.PATH_TEMPLATE, FieldValue.StringField(it))
}
}
}

extraFields.toFields() + fields
}

internal val matchingFields: InternalFieldsMap = headers?.let { HTTPHeaders.normalizeHeaders(it) }.toFields()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

package io.bitdrift.capture.network

import io.bitdrift.capture.CaptureJniLibrary
import io.bitdrift.capture.InternalFieldsMap
import io.bitdrift.capture.events.span.SpanField
import io.bitdrift.capture.providers.FieldValue
Expand Down Expand Up @@ -36,60 +35,75 @@ data class HttpResponseInfo @JvmOverloads constructor(
) {
internal val name: String = "HTTPResponse"

// Lazy since the creation of the `HTTPResponseInfo` can happen before the logger is initialized
// and trying to call a native `CaptureJniLibrary.normalizeUrlPath` method before the `Capture`
// library is loaded leads to unsatisfied linking error.
internal val fields: InternalFieldsMap by lazy {
val fields = buildMap {
put(SpanField.Key.TYPE, FieldValue.StringField(SpanField.Value.TYPE_END))
put(SpanField.Key.DURATION, FieldValue.StringField(durationMs.toString()))
put(SpanField.Key.RESULT, FieldValue.StringField(response.result.name.lowercase()))
putOptional("_status_code", response.statusCode)
putOptional("_error_type", response.error) { it::javaClass.get().simpleName }
putOptional("_error_message", response.error) { it.message.orEmpty() }
putOptional(HttpFieldKey.HOST, response.host)
putOptional(HttpFieldKey.PATH, response.path?.value)
putOptional(HttpFieldKey.QUERY, response.query)
internal val fields: InternalFieldsMap =
run {
val fields = buildMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

this code is very hard to reason about, can you add some comments about what you're trying to achieve here?

this.put(SpanField.Key.TYPE, FieldValue.StringField(SpanField.Value.TYPE_END))
this.put(
SpanField.Key.DURATION,
FieldValue.StringField(durationMs.toString()),
)
this.put(
SpanField.Key.RESULT,
FieldValue.StringField(response.result.name.lowercase()),
)
putOptional("_status_code", response.statusCode)
putOptional(
"_error_type",
response.error,
) { it::javaClass.get().simpleName }
putOptional(
"_error_message",
response.error,
) { it.message.orEmpty() }
putOptional(HttpFieldKey.HOST, response.host)
putOptional(HttpFieldKey.PATH, response.path?.value)
putOptional(HttpFieldKey.QUERY, response.query)

response.path?.let {
val requestPathTemplate = if (request.path?.value == it.value) {
// If the path between request and response did not change and an explicit path
// template was provided as part of a request use it as path template on a response.
request.path.template
} else {
null
}
response.path?.let {
val requestPathTemplate =
if (request.path?.value == it.value) {
// If the path between request and response did not change and an explicit path
// template was provided as part of a request use it as path template on a response.
request.path.template
} else {
null
}

@Suppress("SwallowedException")
val normalized: String? = requestPathTemplate?.let { it }
?: it.template?.let { it }
?: try {
CaptureJniLibrary.normalizeUrlPath(it.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's great to be able to get rid of this call

} catch (e: Throwable) {
null
}
@Suppress("SwallowedException")
val normalized: String? = requestPathTemplate?.let { it }
?: it.template?.let { it }

putOptional(HttpFieldKey.PATH_TEMPLATE, normalized)
}
putOptional(HttpFieldKey.PATH_TEMPLATE, normalized)
}

metrics?.let {
put("_request_body_bytes_sent_count", FieldValue.StringField(it.requestBodyBytesSentCount.toString()))
put("_response_body_bytes_received_count", FieldValue.StringField(it.responseBodyBytesReceivedCount.toString()))
put("_request_headers_bytes_count", FieldValue.StringField(it.requestHeadersBytesCount.toString()))
put("_response_headers_bytes_count", FieldValue.StringField(it.responseHeadersBytesCount.toString()))
putOptional("_dns_resolution_duration_ms", it.dnsResolutionDurationMs)
metrics?.let<HttpRequestMetrics, Unit> {
this.put(
"_request_body_bytes_sent_count",
FieldValue.StringField(it.requestBodyBytesSentCount.toString()),
)
this.put(
"_response_body_bytes_received_count",
FieldValue.StringField(it.responseBodyBytesReceivedCount.toString()),
)
this.put(
"_request_headers_bytes_count",
FieldValue.StringField(it.requestHeadersBytesCount.toString()),
)
this.put(
"_response_headers_bytes_count",
FieldValue.StringField(it.responseHeadersBytesCount.toString()),
)
putOptional("_dns_resolution_duration_ms", it.dnsResolutionDurationMs)
}
}
// If the path between request and response did not change and an explicit path
// template was provided as part of a request use it as path template on a response.
extraFields.toFields() + request.commonFields + fields
}

extraFields.toFields() + request.commonFields + fields
}

// Lazy since the creation of the `HTTPResponseInfo` can happen before the logger is initialized
// and trying to call a native `CaptureJniLibrary.normalizeUrlPath` method before the `Capture`
// library is loaded leads to unsatisfied linking error.
internal val matchingFields: InternalFieldsMap by lazy {
internal val matchingFields: InternalFieldsMap =
request.fields.mapKeys { "_request.${it.key}" } +
request.matchingFields.mapKeys { "_request.${it.key}" } +
response.headers?.let { HTTPHeaders.normalizeHeaders(it) }.toFields()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ class CaptureLoggerTest {
"_host" to "api.bitdrift.io",
"_method" to "GET",
"_path" to "/my_path/12345",
"_path_template" to "/my_path/<id>",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we now remove the CaptureJniLibrary.load() and maybe even make it a plain unit test (vs robolectric one)?

Copy link
Contributor Author

@Augustyniak Augustyniak Aug 28, 2024

Choose a reason for hiding this comment

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

not this particular test, it starts a test server and does depend on some native code. I removed CaptureJniLibrary.load from three other places tho.

"_query" to "my=query",
"_span_id" to spanId.toString(),
"_span_name" to "_http",
Expand Down Expand Up @@ -164,7 +163,6 @@ class CaptureLoggerTest {
"_host" to "api.bitdrift.io",
"_method" to "GET",
"_path" to "/my_path/12345",
"_path_template" to "/my_path/<id>",
"_query" to "my=query",
"_span_id" to spanId.toString(),
"_span_name" to "_http",
Expand All @@ -181,7 +179,6 @@ class CaptureLoggerTest {
"_request._host" to "api.bitdrift.io",
"_request._method" to "GET",
"_request._path" to "/my_path/12345",
"_request._path_template" to "/my_path/<id>",
"_request._span_id" to spanId.toString(),
"_request._span_name" to "_http",
"_request._span_type" to "start",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ import org.junit.Test
import java.util.UUID

class HttpRequestInfoTest {
init {
CaptureJniLibrary.load()
}

@Test
fun testFields() {
Expand Down Expand Up @@ -54,38 +51,4 @@ class HttpRequestInfoTest {
).toFields(),
)
}

@Test
fun testTemplateFallback() {
val spanId = UUID.randomUUID()
val requestInfo = HttpRequestInfo(
host = "api.bitdrift.io",
method = "GET",
path = HttpUrlPath("/my_path/12345"),
query = "my=query",
headers = mapOf("content-type" to "json"),
spanId = spanId,
extraFields = mapOf("my_extra_key_1" to "my_extra_value_1"),
)

assertThat(requestInfo.fields).isEqualTo(
mapOf(
"_host" to "api.bitdrift.io",
"_method" to "GET",
"_path" to "/my_path/12345",
"_path_template" to "/my_path/<id>",
"_query" to "my=query",
"_span_id" to spanId.toString(),
"_span_name" to "_http",
"_span_type" to "start",
"my_extra_key_1" to "my_extra_value_1",
).toFields(),
)

assertThat(requestInfo.matchingFields).isEqualTo(
mapOf(
"_headers.content-type" to "json",
).toFields(),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ import org.junit.Test
import java.util.UUID

class HttpResponseInfoTest {
init {
CaptureJniLibrary.load()
}

@Test
fun test_response_template_override() {
Expand Down Expand Up @@ -67,7 +64,7 @@ class HttpResponseInfoTest {
}

@Test
fun test_http_request_explicit_path_template() {
fun testHTTPRequestExplicitPathTemplate() {
val spanId = UUID.randomUUID()
val requestInfo = HttpRequestInfo(
host = "foo.com",
Expand Down Expand Up @@ -129,7 +126,6 @@ class HttpResponseInfoTest {
"_host" to "api.bitdrift.io",
"_method" to "GET",
"_path" to "/my_path/12345",
"_path_template" to "/my_path/<id>",
"_query" to "my=query",
"_span_id" to spanId.toString(),
"_span_name" to "_http",
Expand All @@ -156,7 +152,6 @@ class HttpResponseInfoTest {
"_host" to "foo.com",
"_method" to "GET",
"_path" to "/foo_path/12345",
"_path_template" to "/foo_path/<id>",
"_query" to "foo_query",
"_span_id" to spanId.toString(),
"_span_name" to "_http",
Expand All @@ -175,7 +170,6 @@ class HttpResponseInfoTest {
"_request._host" to "api.bitdrift.io",
"_request._method" to "GET",
"_request._path" to "/my_path/12345",
"_request._path_template" to "/my_path/<id>",
"_request._span_id" to spanId.toString(),
"_request._span_name" to "_http",
"_request._span_type" to "start",
Expand Down
1 change: 0 additions & 1 deletion platform/jvm/jni_symbols.lds
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,5 @@ Java_io_bitdrift_capture_CaptureJniLibrary_flush
Java_io_bitdrift_capture_CaptureJniLibrary_debugDebug
Java_io_bitdrift_capture_CaptureJniLibrary_debugError
Java_io_bitdrift_capture_CaptureJniLibrary_reportError
Java_io_bitdrift_capture_CaptureJniLibrary_normalizeUrlPath
Java_io_bitdrift_capture_Jni_isRuntimeEnabled
JNI_OnLoad
23 changes: 1 addition & 22 deletions platform/jvm/src/jni.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use jni::signature::{Primitive, ReturnType};
use jni::sys::{jboolean, jbyteArray, jdouble, jint, jlong, jobject, jvalue, JNI_TRUE};
use jni::{JNIEnv, JavaVM};
use platform_shared::metadata::Mobile;
use platform_shared::{normalize_url_path, LoggerHolder, LoggerId};
use platform_shared::{LoggerHolder, LoggerId};
use std::borrow::{Borrow, Cow};
use std::collections::HashMap;
use std::ffi::c_void;
Expand Down Expand Up @@ -1072,27 +1072,6 @@ fn exception_stacktrace(
Ok(Some(stacktrace_str.to_string_lossy().to_string()))
}

#[no_mangle]
pub extern "system" fn Java_io_bitdrift_capture_CaptureJniLibrary_normalizeUrlPath<'a>(
env: JNIEnv<'a>,
_class: JClass<'_>,
url_path: JString<'a>,
) -> JString<'a> {
bd_client_common::error::with_handle_unexpected_or(
|| {
let url_path = unsafe { env.get_string_unchecked(&url_path) }?
.to_string_lossy()
.to_string();

let normalized_url_path = normalize_url_path(&url_path);

Ok(env.new_string(normalized_url_path)?)
},
unsafe { JString::from_raw(core::ptr::null_mut()) },
"jni normalize_url_path",
)
}

#[no_mangle]
pub extern "system" fn Java_io_bitdrift_capture_Jni_isRuntimeEnabled(
env: JNIEnv<'_>,
Expand Down
10 changes: 1 addition & 9 deletions platform/shared/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,14 @@

pub mod error;
pub mod metadata;
pub mod url_normalizer;

use bd_client_common::error::handle_unexpected;
use bd_runtime::runtime::Snapshot;
use std::future::Future;
use std::ops::Deref;
use std::pin::Pin;
use std::sync::{Arc, LazyLock};
use url_normalizer::URLNormalizer;
use std::sync::Arc;

static URL_NORMALIZER: LazyLock<URLNormalizer> = LazyLock::new(URLNormalizer::default);

/// This is the logger ID that is passed to the platform code. It is a typed wrapper around an i64
/// that encodes the pointer to the `LoggerHolder` object.
Expand Down Expand Up @@ -168,8 +165,3 @@ impl<'a> From<LoggerId<'a>> for i64 {
logger.value
}
}

#[must_use]
pub fn normalize_url_path(url_path: &str) -> String {
URL_NORMALIZER.normalize(url_path)
}
Loading
Loading