Skip to content

Small optimization in LoggerImpl.extractFields #529

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.

Sign up for GitHub

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 2 commits into from
Jul 28, 2025
Merged

Small optimization in LoggerImpl.extractFields #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99b181e
Small optimization in LoggerImpl.extractFields
pyricau Jul 28, 2025
485dedb
Rebase + format
FranAguilera Jul 28, 2025
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
36 changes: 27 additions & 9 deletions platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/LoggerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,16 +468,34 @@ internal class LoggerImpl(
internal fun extractFields(
fields: Map<String, String>?,
throwable: Throwable?,
): InternalFieldsMap? =
buildMap {
fields?.let {
putAll(it)
}
throwable?.let {
put("_error", it.javaClass.name.orEmpty())
put("_error_details", it.message.orEmpty())
): InternalFieldsMap {
// Maintainer note: keep initialCapacity in sync with the code adding fields to the map.
val initialCapacity = (fields?.size ?: 0) + (throwable?.let { 2 } ?: 0)
Copy link
Contributor Author

@pyricau pyricau Jul 28, 2025

Choose a reason for hiding this comment

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

note: the 2 here has to be kept in sync with the code in throwable?.let {}. That's slightly annoying, an alternative could be:

companion object {
  val throwableExtractors = listOf<(Throwable) -> Pair<String, FieldValue>>({
    "_error" to it.javaClass.name.orEmpty().toFieldValue()
  }, {
    "_error_details" to it.message.orEmpty().toFieldValue()
  })
}

then:

val initialCapacity = (fields?.size ?: 0) + (throwable?.let { throwableExtractors.size } ?: 0)

throwable?.let {
  for (extractor in throwableExtractors) {
    val (key, value) = extractor(throwable)
    extractedFields[key] = value
  }

Copy link
Contributor

@FranAguilera FranAguilera Jul 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I like this alternative better over the hardcoded 2. The one relying on 2 feels fragile over time and easy to forget if there is a need to add another _error_... fields in the future, though no sure it's worth the cost for creating throwableExtractors

if (initialCapacity == 0) {
// If throwable is null AND fields is either null or empty, no need to create a HashMap.
return emptyMap()
}
// Create a hashmap of the exact target size and with the right final value type, instead
// of creating a temporary map and then converting it with Map.toFields()
val extractedFields = HashMap<String, FieldValue>(initialCapacity)
fields?.let {
for ((key, value) in it) {
// Java interop: clients could have passed in null keys or values.
@Suppress("SENSELESS_COMPARISON")
if (key != null && value != null) {
extractedFields[key] = value.toFieldValue()
}
}
}.toFields()
}
throwable?.let {
extractedFields["_error"] =
it.javaClass.name
.orEmpty()
.toFieldValue()
extractedFields["_error_details"] = it.message.orEmpty().toFieldValue()
}
return extractedFields
}

internal fun flush(blocking: Boolean) {
CaptureJniLibrary.flush(this.loggerId, blocking)
Expand Down
2 changes: 1 addition & 1 deletion platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/providers/FieldProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ internal fun ByteArray.toFieldValue() =
*/
@Suppress("SENSELESS_COMPARISON")
internal fun Map<String, String>?.toFields(): Map<String, FieldValue> {
if (this == null) return emptyMap()
if (isNullOrEmpty()) return emptyMap()
Copy link
Contributor

@FranAguilera FranAguilera Jul 28, 2025

Choose a reason for hiding this comment

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

good catch


val result = HashMap<String, FieldValue>(size)
for ((key, value) in this) {
Expand Down
Loading