matomo-org / matomo-sdk-ios

Matomo iOS, tvOS and macOS SDK: a Matomo tracker written in Swift
MIT License
390 stars 164 forks source link

cpu used is greater than 90% #117

Closed meoliver closed 7 years ago

meoliver commented 7 years ago

hello,When the app is I use this, very high CPU。Especially hot lead to equipment。

This thing when I commented code, this phenomenon is gone. ex: //PiwikTracker // [PiwikTracker sharedInstanceWithSiteID:PiwikProductionSiteId baseURL:[NSURL URLWithString:PiwikProductionServerURL]]; // [PiwikTracker sharedInstance].includeDefaultCustomVariable = NO; // [PiwikTracker sharedInstance].dispatchInterval = 5;

brototyp commented 7 years ago

Hi @meoliver, thanks for your report. Can you record an instrument session with the Time Profiler so we can debug which code is actually causing the load?

meoliver commented 7 years ago

I have to reason, but some question, need you to explain.

When the request error ,call back, class PiwikNSURLSessionDispatcher method shouldAbortdispatchForNetworkError when return yes( error.code == NSURLErrorCannotFindHost )。 Then call Class PiwikTracker method sendEvent(failureBlock line: 1033),and hasMore is yes。
This time, the request will be continuous, leading to excessive CPU load。

Call Class PiwikTracker method eventsFromStore: completionBlock: (line 1490 ), completionBlock parameter hasMore may be yes。

I want to know Class PiwikTracker line1461 -- line 1490 fetchRequest.fetchLimit、returnCount、eventEntities.count ,Their calculation logic is to do?

thanks

meoliver commented 7 years ago

@brototyp If you are paying attention to my question?

brototyp commented 7 years ago

Hi @meoliver, thanks for digging into the issue. As far as I understand you: When there is an issue transmitting an event to the server and not all events can be transmitted with one call to the server, the SDK will try to transmit the same event(s) over and over. If the error is always the same it will always repeat to transmit the same event over and over again.

As far as I can see this should be prevented by the shouldContinue value in the failure completion block. The 'PiwikNSURLSessionDispatcher.m' has a '-shouldAbortdispatchForNetworkError:' method that should return yes for a NSURLErrorCannotFindHost error. This matches with what you found out. I am not sure why in your case this leads to excessive CPU load.

Did you were able to record an Instruments session with the Time Profiler? Unfortunately I am not sure what exactly your question is. Can you rephrase it?

meoliver commented 7 years ago

@brototyp thanks。

when '-shouldAbortdispatchForNetworkError:' return yes,and call 'eventsFromStore: completionBlock: '

When the core data, save the data after the article has more than 20.

numberOfEvents is 20; method ’eventsFromStore:(NSUInteger)numberOfEvents completionBlock: ‘ {


completionBlock(entityIDs, events, eventEntities.count == fetchRequest.fetchLimit ? YES : NO);

// this ‘eventEntities.count == fetchRequest.fetchLimit‘ is yes。


}

NSURLErrorCannotFindHost error ,There has been。

method 'sendEvent '

void (^failureBlock)(BOOL shouldContinue) = ^ (BOOL shouldContinue) { PiwikDebugLog(@"Failed to send stats to Piwik server");

    if (shouldContinue) {

*** this code pay attention to** [weakSelf sendEventDidFinishHasMorePending:hasMore];

// method ’sendEventDidFinishHasMorePending:' hasMore is yes, call ’sendEvent‘,but NSURLErrorCannotFindHost error,He'll cycle call。

// ’sendEvent‘----’eventsFromStore:completionBlock:‘----failureBlock() ----- ’sendEvent‘

*** this code pay attention to** } else { [weakSelf sendEventDidFinishHasMorePending:NO]; }

  }
brototyp commented 7 years ago

@meoliver I am still not sure what you are meaning. Let me try to elaborate:

Is this correct? Did you were able to record an Instruments session with the Time Profiler? Please attach a Time Profiler session.

meoliver commented 7 years ago

@brototyp thanks。

[PiwikTracker sharedInstanceWithSiteID:PiwikProductionSiteId baseURL:[NSURL URLWithString:@"http://tracking.googlerror.com/piwik/"]];
[PiwikTracker sharedInstance].includeDefaultCustomVariable = NO;
[PiwikTracker sharedInstance].dispatchInterval = 5;

The @"http://tracking.googlerror.com/piwik/" is NSURLErrorCannotFindHost

the failure-block (PiwikTracker:1033) add nslog(),For example:

if (shouldContinue) { NSLog(@"Piwik has been called"); [weakSelf sendEventDidFinishHasMorePending:hasMore]; } else { [weakSelf sendEventDidFinishHasMorePending:NO]; }

and Click on the for more than 21 times。look the log

meoliver commented 7 years ago

@brototyp Hi,this is time profiler. Thanks piwik.zip

brototyp commented 7 years ago

Hi @meoliver, thanks for the recording. I was able to reproduce the issue and I guess I know what the issue is: PiwikNSURLSessionDispatcher:shouldAbortdispatchForNetworkError retunes YES if dispatching should be aborted. This method is used in PiwikNSURLSessionDispatcher:97. But its value is used as a parameter for the faliure-blocks parameter shouldContinue. That is exactly the wrong thing. If I see it correct line 97 should be: failureBlock(![self shouldAbortdispatchForNetworkError:error]); so inverting the bool value. Can you try that and get back in touch with me?

meoliver commented 7 years ago

@brototyp thanks failureBlock(![self shouldAbortdispatchForNetworkError:error]); I think it will not solve the problem。 when NSURLErrorTimedOut,Still can continuous to send the request。

failureBlock(NO); This will avoid, but it's meaningless to parameters.

meoliver commented 7 years ago

@brototyp Hi, I want to know Class ‘PiwikTracker’ line1461 to line 1490 fetchRequest.fetchLimit、returnCount、eventEntities.count ,Their calculation logic is to do?

line 1490 completionBlock(entityIDs, events, eventEntities.count == fetchRequest.fetchLimit ? YES : NO);

The third parameter is yes,and shouldAbortdispatchForNetworkError: return yes 。This will call cycles。

thanks

brototyp commented 7 years ago

I think that is expected behavior. The sdk continues trying to transmit events if there is a timeout. We expect it to just be a temporary issue. Do you know what your timeout duration is? Here the timeout duration decides the "looping". So if the timeout is 1s the sdk will loop once per second. Which is an issue in the architecture. Better would be: If an event can't be transmitted in a transmission run we should not retry this event in this run. That is a bigger issue and requires quite some refactoring so we only fetch events we haven't already fetched.

I am not sure what your question there is. The eventEntities.count == fetchRequest.fetchLimit ? YES : NO is meant to decide it there are (or might be) more events queued. There is an issue here, that if there are exactly the amount of events queued that we want to transmit this will try one more time but I guess thats fine.

meoliver commented 7 years ago

@brototyp Thank you very much care about my question. So, this code you will follow the new to GitHub? failureBlock(![self shouldAbortdispatchForNetworkError:error]);

Or,I modify the native code?

brototyp commented 7 years ago

Please try to change it locally in your code. We are currently rewriting the whole sdk, fully written in swift. We are making sure this bug doesn't exist in the new sdk.

tkapler commented 7 years ago

@meoliver - do you have a patch for this problem? We face it as well and we cannot wait for swift version. Could you please send it to me (tomas.kapler@etnetera.cz). Thank you.

brototyp commented 7 years ago

@tkapler Have you tried the patch I proposed? If so, can you give us some feedback if it changed anything or if it did not affect the issue. I did interpret the silence @meoliver as approval that the patch works.

brototyp commented 7 years ago

Closing this for now. Feel free to reopen if you have more information on this topic.