Consolidate logging #19
Conversation
| [loggingWrapper.logger logEventAction:KLogActionStopScreensharing | ||
| variation:KLogVariationAttempt | ||
| completion:nil]; | ||
| } |
There was a problem hiding this comment.
Under screen sharing, it will log both. Should we put KLogActionStopPublishing to the else branch?
There was a problem hiding this comment.
@Lucashuang0802 in Android, we are following the approach below:
- start/stop publishing means starting to share your media independently on if it is screen or not.
- start/stop screensharing means you are publishing and also, it is the screensharing case.
So the stopPublishing log should not be in the else, it is covering the ss case as well. wdyt?
There was a problem hiding this comment.
It does not make too much sense to me as it's the same event but different capture type. and I do believe publishing and screen-sharing are same but capturing from a different feed. that will probably create confusion.
There was a problem hiding this comment.
After review it, I agree. Maybe, it could be confused, so, we are going to log:
- start/stop publishing media (camera stream)
- start/stop screensharing
I will add the modification to android core as well. thanks!!
| static NSString* const KLogClientVersion = @"ios-vsol-2.0.0"; | ||
| static NSString* const kLogComponentIdentifier = @"OTAcceleratorCore"; | ||
| static NSString* const KLogClientVersion = @"ios-vsol-1.0.4"; | ||
| static NSString* const kLogComponentIdentifier = @"acceleratorCore"; |
There was a problem hiding this comment.
Let's use upper case K for making it consistent.
| NSLog(@"%s: %@", __PRETTY_FUNCTION__, error); | ||
| } | ||
| [subscriberObject.subscriberView removeFromSuperview]; | ||
| [subscriberObject.subscriberView clean]; | ||
| subscriberObject.subscriber = nil; | ||
| subscriberObject.subscriberView = nil; | ||
| [loggingWrapper.logger logEventAction:KLogActionRemoveRemote | ||
| variation:KLogVariationSuccess | ||
| completion:nil]; |
There was a problem hiding this comment.
Here, we will have a situation to log success and failure the same time.
There was a problem hiding this comment.
@Lucashuang0802 you are right, the error is not a return. I will add the success case in the else.
| connectionId:session.connection.connectionId | ||
| partnerId:@([self.session.apiKey integerValue])]; | ||
| [loggingWrapper.logger logEventAction:KLogActionConnect | ||
| variation:KLogVariationSuccess | ||
| completion:nil]; |
There was a problem hiding this comment.
This is a duplicate log which is captured in connect method. That's why I only use StartComm and StopComm
There was a problem hiding this comment.
You are right. Let us use the success case in the callback, because we have the connectionId, so the log will be more complete.
| [loggingWrapper.logger logEventAction:KLogActionStartScreensharing | ||
| variation:KLogVariationFailure | ||
| completion:nil]; | ||
| } |
There was a problem hiding this comment.
Under screen sharing, it will log both. Should we put KLogActionStartPublishing to the else branch?
There was a problem hiding this comment.
Same we commented for multiparty case
| [[LoggingWrapper sharedInstance].logger logEventAction:KLogActionStartScreensharing | ||
| variation:KLogVariationSuccess | ||
| completion:nil]; | ||
| } |
There was a problem hiding this comment.
Under screen sharing, it will log both. Should we put KLogActionStartPublishing to the else branch?
| variation:KLogVariationFailure | ||
| completion:nil]; | ||
| } | ||
| return subscriberError; |
There was a problem hiding this comment.
This method, we did not log success.
There was a problem hiding this comment.
the success case is in the callback: subscriberDidConnectToStream, wdyt?
|
Let's see whether we can simplify the entry a little bit as I have fixed this one already. Mostly, issues are on text-chat and annotation. |
|
|
||
| if (self.screenSharingView) { | ||
| [loggingWrapper.logger logEventAction:KLogActionStopScreensharing | ||
| variation:KLogVariationAttempt |
There was a problem hiding this comment.
Why is it the attempt action here?
| if (!_publisher) return; | ||
| _publisher.publishAudio = publishAudio; | ||
| [[MultiPartyLoggingWrapper sharedInstance].logger logEventAction:KLogActionEnableLocalMedia | ||
| variation:KLogVariationSuccess | ||
| completion:nil]; |
There was a problem hiding this comment.
For cases like this, why do we even have attempt if we don't have a case to fail?
|
Please re-consider to simplify logging entries as it increases the codebase dramatically. Also, the client and backend also have the log. Why do we need to have so many entries that are redundant? |
|
@Lucashuang0802 please note, we are adding client logs for the public API methods in the core. The native clients and servers are logging different actions. |
Pull request checklist
This fixes issue #___.
What's in this pull request?