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.

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

Jump to bottom

Added operation duration to the finishing log. #356

Merged
merged 6 commits into from Jan 16, 2018
Merged

Added operation duration to the finishing log. #356

merged 6 commits into from Jan 16, 2018

Conversation

uKL
Copy link
Collaborator

@uKL uKL commented Jan 16, 2018

No description provided.

dariuszseweryn
dariuszseweryn reviewed Jan 16, 2018
View reviewed changes
Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Should we care for performance of these logs?

@uKL
Copy link
Collaborator Author

uKL commented Jan 16, 2018

@dariuszseweryn, refactored a bit.

dariuszseweryn
dariuszseweryn reviewed Jan 16, 2018
View reviewed changes
Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

How does it look like in the actual execution? Doesn't the delegation mess with the log tag generation?

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/util/OperationLogger.java Outdated

public class OperationLogger {

@RestrictTo(RestrictTo.Scope.LIBRARY)
Copy link
Owner

Choose a reason for hiding this comment

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

Not really needed since it is under internal package

@uKL
Copy link
Collaborator Author

uKL commented Jan 16, 2018

Let me check the delegation, you may be right.

@uKL
Copy link
Collaborator Author

uKL commented Jan 16, 2018

Actually no, since it's a static method it works.

01-16 12:19:59.817 19756-19756/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: QUEUED ScanOperationApi21(123696767)
01-16 12:19:59.818 19756-19786/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: STARTED ScanOperationApi21(123696767)
01-16 12:19:59.820 19756-20062/com.polidea.rxandroidble.sample I/RxBle#QueueOperation: Scan operation is requested to start.
01-16 12:19:59.933 19756-19786/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: FINISHED in 115ms ScanOperationApi21(123696767)
01-16 12:20:07.645 21313-21313/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: QUEUED ScanOperationApi21(64820451)
01-16 12:20:07.646 21313-21328/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: STARTED ScanOperationApi21(64820451)
01-16 12:20:07.649 21313-21349/com.polidea.rxandroidble.sample I/RxBle#QueueOperation: Scan operation is requested to start.
01-16 12:20:07.767 21313-21328/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: FINISHED in 121ms ScanOperationApi21(64820451)
01-16 12:20:08.700 21313-21349/com.polidea.rxandroidble.sample I/RxBle#CancellableSubscription: Scan operation is requested to stop.

dariuszseweryn
dariuszseweryn reviewed Jan 16, 2018
View reviewed changes
Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Good. I liked the way it was displayed before so the operation name would consistently be displayed in a single place (%8s) in logs. Could we leave it this way? The finish log could look something like: FINISHED <Operation>(id) took xx ms

Shouldn't the ms should have a space before the amount of milliseconds?

@uKL
Copy link
Collaborator Author

uKL commented Jan 16, 2018

Is this what you meant?

01-16 12:37:17.492 24123-24123/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: QUEUED   ScanOperationApi21(39162453)
01-16 12:37:17.494 24123-24151/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: STARTED  ScanOperationApi21(39162453)
01-16 12:37:17.503 24123-24416/com.polidea.rxandroidble.sample I/RxBle#QueueOperation: Scan operation is requested to start.
01-16 12:37:17.623 24123-24151/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: FINISHED ScanOperationApi21(39162453) in 131 ms
01-16 12:37:20.843 24123-24416/com.polidea.rxandroidble.sample I/RxBle#CancellableSubscription: Scan operation is requested to stop.
01-16 12:37:23.275 24123-25202/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: QUEUED   ConnectOperation(101216245)
01-16 12:37:23.276 24123-24151/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: STARTED  ConnectOperation(101216245)
01-16 12:37:23.282 24123-24416/com.polidea.rxandroidble.sample V/RxBle#BleConnectionCompat: Connecting without reflection
01-16 12:37:23.532 24123-24136/com.polidea.rxandroidble.sample D/RxBle#BluetoothGatt: onConnectionStateChange newState=2 status=0
01-16 12:37:23.534 24123-24151/com.polidea.rxandroidble.sample D/RxBle#ClientOperationQueue: FINISHED ConnectOperation(101216245) in 259 ms

dariuszseweryn
dariuszseweryn approved these changes Jan 16, 2018
View reviewed changes
Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Exactly 👍

.../src/main/java/com/polidea/rxandroidble/internal/serialization/ClientOperationQueueImpl.java Outdated
@@ -1,9 +1,11 @@
package com.polidea.rxandroidble.internal.serialization;

import android.support.annotation.RestrictTo;

Copy link
Owner

Choose a reason for hiding this comment

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

We should include the formatting configuration into the repository so these kind of changes would not happen.

@uKL uKL merged commit cae7cf4 into master Jan 16, 2018
@uKL uKL deleted the feature/operation_performance_logging branch January 16, 2018 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariuszseweryn dariuszseweryn dariuszseweryn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@uKL @dariuszseweryn