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

Bug 1900313 - Bump the string length limit to 255 characters #2857

Merged
merged 2 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion .dictionary
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
personal_ws-1.1 en 282 utf-8
personal_ws-1.1 en 283 utf-8
AAR
AARs
ABI
Expand Down Expand Up @@ -175,6 +175,7 @@ html
iMacs
illumos
init
initializer
inlined
instrumentations
integrations
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

[Full changelog](https://github.com/mozilla/glean/compare/v60.3.0...main)

* General
* Bump the string length limit to 255 characters ([#2857](https://github.com/mozilla/glean/pull/2857))
* Android
* Delay log init until Glean is getting initialized ([#2858](https://github.com/mozilla/glean/pull/2858))
* Update to Gradle v8.8 ([#2860](https://github.com/mozilla/glean/pull/2860))
* Updated Kotlin to version 1.9.24 ([#2861](https://github.com/mozilla/glean/pull/2861))

Expand Down
1 change: 1 addition & 0 deletions docs/dev/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- [Android bindings](android/index.md)
- [Setup Build Environment](android/setup-android-build-environment.md)
- [Android SDK/NDK versions](android/sdk-ndk-versions.md)
- [Logging](android/logging.md)
- [Development with android-components](android/development-with-android-components.md)
- [Locally-published components in Fenix](android/locally-published-components-in-fenix.md)
- [Substituting glean_parser](android/glean-parser-substitution.md)
Expand Down
12 changes: 12 additions & 0 deletions docs/dev/android/logging.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Logging

Logs from `glean-core` are only passed through to the Android logging framework when `Glean.initialize` is called for the first time.
This means any logging that might happen before that, e.g. from early metric collection will not be collected.

If these logs are needed for debugging add the following initializer to `Glean.kt`:

```kotlin
init {
gleanEnableLogging()
}
```
3 changes: 2 additions & 1 deletion docs/user/reference/metrics/string.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ Glean.searchDefault.name.set("wikipedia");

#### Limits

* Fixed maximum string length: 100. Longer strings are truncated. This is measured in the number of bytes when the string is encoded in UTF-8.
* Fixed maximum string length: 255. Longer strings are truncated. This is measured in the number of bytes when the string is encoded in UTF-8.
* Prior to Glean v60.4.0 the limit was 100 bytes.

## Testing API

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,6 @@ open class GleanInternalAPI internal constructor() {

internal var isCustomDataPath: Boolean = false

init {
gleanEnableLogging()
}

/**
* Initialize the Glean SDK.
*
Expand Down Expand Up @@ -185,6 +181,8 @@ open class GleanInternalAPI internal constructor() {
configuration: Configuration = Configuration(),
buildInfo: BuildInfo,
) {
gleanEnableLogging()

configuration.dataPath?.let { safeDataPath ->
// When the `dataPath` is provided, we need to make sure:
// 1. The database path provided is not `glean_data`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class StringMetricTypeTest {
),
)

stringMetric.set("0123456789".repeat(11))
stringMetric.set("0123456789".repeat(26))

assertEquals(1, stringMetric.testGetNumRecordedErrors(ErrorType.INVALID_OVERFLOW))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class StringMetricTests: XCTestCase {
disabled: false
))

stringMetric.set(String(repeating: "0123456789", count: 11))
stringMetric.set(String(repeating: "0123456789", count: 26))

XCTAssertEqual(1, stringMetric.testGetNumRecordedErrors(.invalidOverflow))
}
Expand Down
2 changes: 1 addition & 1 deletion glean-core/python/tests/metrics/test_string.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def test_setting_a_long_string_records_an_error():
)
)

string_metric.set("0123456789" * 11)
string_metric.set("0123456789" * 26)

assert 1 == string_metric.test_get_num_recorded_errors(testing.ErrorType.INVALID_OVERFLOW)

Expand Down
4 changes: 2 additions & 2 deletions glean-core/src/metrics/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::util::truncate_string_at_boundary_with_error;
use crate::CommonMetricData;
use crate::Glean;

const MAX_LENGTH_VALUE: usize = 100;
const MAX_LENGTH_VALUE: usize = 255;

/// A string metric.
///
Expand Down Expand Up @@ -166,7 +166,7 @@ mod test {
dynamic_label: None,
});

let sample_string = "0123456789".repeat(11);
let sample_string = "0123456789".repeat(26);
metric.set_sync(&glean, sample_string.clone());

let truncated = truncate_string_at_boundary(sample_string, MAX_LENGTH_VALUE);
Expand Down
2 changes: 1 addition & 1 deletion glean-core/tests/labeled.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ fn can_record_error_for_submetric() {
);

let metric = labeled.get("label1");
metric.set_sync(&glean, "01234567890".repeat(20));
metric.set_sync(&glean, "01234567890".repeat(26));

// Make sure that the errors have been recorded
assert_eq!(
Expand Down
6 changes: 3 additions & 3 deletions glean-core/tests/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,12 @@ fn long_string_values_are_truncated() {
..Default::default()
});

let test_sting = "01234567890".repeat(20);
metric.set_sync(&glean, test_sting.clone());
let test_string = "01234567890".repeat(26);
metric.set_sync(&glean, test_string.clone());

// Check that data was truncated
assert_eq!(
test_sting[..100],
test_string[..255],
metric.get_value(&glean, "store1").unwrap()
);

Expand Down
Loading