FirePerf: use FPRLogDebug instead of FPRLogError for logging events when FirePerf is disabled #8620
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.
Discussion
Cause/Result
Log level from most verbose to least verbose is:
With those 5, levels,
Notice,Warning, andErrorlogs will show even without-FIRDebugEnabledbeing set. Due to that,FPRLogError(kFPRClientPerfNotConfigured, @"Dropping event since data collection is disabled.");will spam log if gauge metrics are being collected (if they are being collected, they are firing on a timer, like every second). With this PR, only when-FIRDebugEnabledis set, will those log message show up in logs, if you are lucky enough to be selected for sampling in the first place (more on this below).Investigation/Details
Reproducing this issue was difficult at first because of sampling. Long before gauge metrics collection start, in
FPRSessionManager.updateSessionId, there is a check calledisGaugeCollectionEnabledForSessionIdthat decides if current session should be sampled, around 1% chance. Even if selected for sampling, gauge metrics collection usually will not be started at all when FirePerf is disabled (seeFPRGaugeManager.gaugeCollectionEnabled), so no log should appear. Only during cold start, due to technical reasons (too slow to check RemoteConfig), will gauge metrics collection always start. In that case, we must drop gauge events at the dispatch stage, which is where the "Dropping event..." is being logged.In the linked issue above, it is referring to an older issue, in which the author said the log spam was solved by changing
FIRInfoEnabledtoFIRDebugEnabled. That makes no sense becauseDebugis higher verbosity thanInfo. I suspect the author was just lucky (pretty hard to be unlucky since sampling at 1% rate) that their next app-run's sessions weren't selected for sampling.Other possible issues
There is a weird behaviour I ran into withWith further testing, I don't thinkFIRLogInfoandFIRInfoEnabledin that it does not log its own level at allFIRInfoEnabledis even a flag at all, or any ofFIRNoticeEnabled,FIRWarningEnabled, andFIRErrorEnabled. I don't think they have any effect on logging. I can only confirm the following behaviors on my machine:Notice, Warning, Errorlogs when-FIRDebugEnabledis not set,Debug, Info, Notice, Warning, Errorlogs when-FIRDebugEnabledis set.Either
FIRDebugEnabledis the only flag, or something is wrong with FIRLogging, which would be a whole separate issue with another team.