Logcat fixes #250
Merged
Logcat fixes #250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tekjar
reviewed
Jun 24, 2023
uplink/src/collector/logcat.rs
Outdated
| @@ -133,14 +134,18 @@ impl LogEntry { | |||
| } | |||
|
|
|||
| pub fn to_payload(&self, sequence: u32) -> anyhow::Result<Payload> { | |||
| let payload = serde_json::to_value(self)?; | |||
|
|
|||
| Ok(Payload { | |||
| stream: "logs".to_string(), | |||
| device_id: None, | |||
| sequence, | |||
| timestamp: self.timestamp, | |||
There was a problem hiding this comment.
This should be log's timestamp if available
There was a problem hiding this comment.
If logs are expected to have timestamps, can we treat timestamp errors as failures?
There was a problem hiding this comment.
We only read real time logcat output so the two timestamps will always be very close.
There was a problem hiding this comment.
Logcat line regex specifies time field as mandatory so if timestamp isn't there the regex match will fail.
There was a problem hiding this comment.
Flush timeouts are working for logs stream now so the logs issue reactlabs saw shouldn't happen.
tekjar
reviewed
Jun 24, 2023
uplink/src/collector/logcat.rs
Outdated
|
|
||
| fn read_line_lossy(reader: &mut BufReader<ChildStdout>) -> Option<String> { | ||
| let mut buf = Vec::with_capacity(256); | ||
| if let Err(_) = reader.read_until(u8::try_from('\n').unwrap(), &mut buf) { |
There was a problem hiding this comment.
- Change to
reader.read_until(b'\n', &mut buf)? - Change
OptiontoResultand break with error log
tekjar
approved these changes
Aug 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #
Changes
Remove duplicate timestamps in logcat
Handle encoding errors properly in logcat output
@tekjar Doesn't include debugging changes in react-stability
Why?
Trials Performed