Skip to content

Commit

Permalink
Allow image loaders to enable/disable image telemetry
Browse files Browse the repository at this point in the history
Summary:
When shouldEnableLoggingForRequestUrl is false, ImageTelemetry is not initialized, and no logging is done.

* Replace `- (NSString *)loaderModuleNameForRequestUrl:(NSURL *)url` with `- (BOOL)shouldEnableLoggingForRequestUrl:(NSURL *)url`
* Rename RCTImageLoaderInstrumentableProtocol.h -> RCTImageLoaderLoggableProtocol.h

Reviewed By: fkgozali

Differential Revision: D24523984

fbshipit-source-id: a5463eceea1c40f9452b0ad2ee6bf047f71a02c1
  • Loading branch information
p-sun authored and facebook-github-bot committed Oct 30, 2020
1 parent 1b71ec4 commit e37708d
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 45 deletions.
4 changes: 2 additions & 2 deletions Libraries/Image/RCTImageLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
#import <React/RCTImageURLLoader.h>
#import <React/RCTImageCache.h>
#import <React/RCTImageLoaderProtocol.h>
#import <React/RCTImageLoaderInstrumentableProtocol.h>
#import <React/RCTImageLoaderLoggable.h>

@interface RCTImageLoader : NSObject <RCTBridgeModule, RCTImageLoaderProtocol, RCTImageLoaderInstrumentableProtocol>
@interface RCTImageLoader : NSObject <RCTBridgeModule, RCTImageLoaderProtocol, RCTImageLoaderLoggableProtocol>
- (instancetype)init;
- (instancetype)initWithRedirectDelegate:(id<RCTImageRedirectProtocol>)redirectDelegate NS_DESIGNATED_INITIALIZER;
- (instancetype)initWithRedirectDelegate:(id<RCTImageRedirectProtocol>)redirectDelegate
Expand Down
8 changes: 4 additions & 4 deletions Libraries/Image/RCTImageLoader.mm
Original file line number Diff line number Diff line change
Expand Up @@ -839,12 +839,12 @@ - (RCTImageURLLoaderRequest *)loadImageWithURLRequest:(NSURLRequest *)imageURLRe
return [[RCTImageURLLoaderRequest alloc] initWithRequestId:loaderRequest.requestId imageURL:imageURLRequest.URL cancellationBlock:cancellationBlock];
}

- (NSString *)loaderModuleNameForRequestUrl:(NSURL *)url {
- (BOOL)shouldEnablePerfLoggingForRequestUrl:(NSURL *)url {
id<RCTImageURLLoader> loadHandler = [self imageURLLoaderForURL:url];
if ([loadHandler respondsToSelector:@selector(loaderModuleNameForRequestUrl:)]) {
return [(id<RCTImageURLLoaderWithAttribution>)loadHandler loaderModuleNameForRequestUrl:url];
if ([loadHandler respondsToSelector:@selector(shouldEnablePerfLogging)]) {
return [(id<RCTImageURLLoaderWithAttribution>)loadHandler shouldEnablePerfLogging];
}
return nil;
return NO;
}

- (void)trackURLImageVisibilityForRequest:(RCTImageURLLoaderRequest *)loaderRequest imageView:(UIView *)imageView
Expand Down
15 changes: 0 additions & 15 deletions Libraries/Image/RCTImageLoaderInstrumentableProtocol.h

This file was deleted.

30 changes: 30 additions & 0 deletions Libraries/Image/RCTImageLoaderLoggable.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* The image loader (i.e. RCTImageLoader) implement this to declare whether image performance should be logged.
*/
@protocol RCTImageLoaderLoggableProtocol

/**
* Image instrumentation - declares whether its caller should log images
*/
- (BOOL)shouldEnablePerfLoggingForRequestUrl:(NSURL *)url;

@end

/**
* Image handlers in the image loader implement this to declare whether image performance should be logged.
*/
@protocol RCTImageLoaderLoggable

/**
* Image instrumentation - declares whether its caller should log images
*/
- (BOOL)shouldEnablePerfLogging;

@end
3 changes: 1 addition & 2 deletions Libraries/Image/RCTImageLoaderWithAttributionProtocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

#import <React/RCTImageLoaderProtocol.h>
#import <React/RCTImageURLLoaderWithAttribution.h>
#import <React/RCTImageLoaderInstrumentableProtocol.h>

RCT_EXTERN BOOL RCTImageLoadingInstrumentationEnabled(void);
RCT_EXTERN BOOL RCTImageLoadingPerfInstrumentationEnabled(void);
Expand All @@ -19,7 +18,7 @@ RCT_EXTERN void RCTEnableImageLoadingPerfInstrumentation(BOOL enabled);
RCT_EXTERN BOOL RCTGetImageLoadingPerfInstrumentationForFabricEnabled();
RCT_EXTERN void RCTSetImageLoadingPerfInstrumentationForFabricEnabledBlock(BOOL (^getEnabled)());

@protocol RCTImageLoaderWithAttributionProtocol<RCTImageLoaderProtocol, RCTImageLoaderInstrumentableProtocol>
@protocol RCTImageLoaderWithAttributionProtocol<RCTImageLoaderProtocol, RCTImageLoaderLoggableProtocol>

// TODO (T61325135): Remove C++ checks
#ifdef __cplusplus
Expand Down
4 changes: 2 additions & 2 deletions Libraries/Image/RCTImageURLLoaderWithAttribution.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

#import <React/RCTImageURLLoader.h>
#import <React/RCTImageLoaderProtocol.h>
#import <React/RCTImageLoaderInstrumentableProtocol.h>
#import <React/RCTImageLoaderLoggable.h>

// TODO (T61325135): Remove C++ checks
#ifdef __cplusplus
Expand Down Expand Up @@ -39,7 +39,7 @@ struct ImageURLLoaderAttribution {
* Same as the RCTImageURLLoader interface, but allows passing in optional `attribution` information.
* This is useful for per-app logging and other instrumentation.
*/
@protocol RCTImageURLLoaderWithAttribution <RCTImageURLLoader, RCTImageLoaderInstrumentableProtocol>
@protocol RCTImageURLLoaderWithAttribution <RCTImageURLLoader, RCTImageLoaderLoggable>

// TODO (T61325135): Remove C++ checks
#ifdef __cplusplus
Expand Down
8 changes: 0 additions & 8 deletions ReactCommon/react/renderer/imagemanager/ImageTelemetry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,6 @@ SurfaceId ImageTelemetry::getSurfaceId() const {
return surfaceId_;
}

std::string ImageTelemetry::getLoaderModuleName() const {
return loaderModuleName_;
}

void ImageTelemetry::setLoaderModuleName(std::string const &loaderModuleName) {
loaderModuleName_ = loaderModuleName;
}

TelemetryTimePoint ImageTelemetry::getWillRequestUrlTime() const {
assert(willRequestUrlTime_ != kTelemetryUndefinedTimePoint);
return willRequestUrlTime_;
Expand Down
3 changes: 0 additions & 3 deletions ReactCommon/react/renderer/imagemanager/ImageTelemetry.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,11 @@ class ImageTelemetry final {
TelemetryTimePoint getWillRequestUrlTime() const;

SurfaceId getSurfaceId() const;
std::string getLoaderModuleName() const;
void setLoaderModuleName(std::string const &loaderModuleName);

private:
TelemetryTimePoint willRequestUrlTime_{kTelemetryUndefinedTimePoint};

const SurfaceId surfaceId_;
std::string loaderModuleName_{""};
};

} // namespace react
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,22 +40,22 @@ - (ImageRequest)requestImage:(ImageSource)imageSource surfaceId:(SurfaceId)surfa
{
SystraceSection s("RCTImageManager::requestImage");

auto telemetry = std::make_shared<ImageTelemetry>(surfaceId);
telemetry->willRequestUrl();
NSURLRequest *request = NSURLRequestFromImageSource(imageSource);
std::shared_ptr<ImageTelemetry> telemetry;
if ([self->_imageLoader shouldEnablePerfLoggingForRequestUrl:request.URL]) {
telemetry = std::make_shared<ImageTelemetry>(surfaceId);
telemetry->willRequestUrl();
} else {
telemetry = nullptr;
}

auto imageRequest = ImageRequest(imageSource, telemetry);
auto weakObserverCoordinator =
(std::weak_ptr<const ImageResponseObserverCoordinator>)imageRequest.getSharedObserverCoordinator();

auto sharedCancelationFunction = SharedFunction<>();
imageRequest.setCancelationFunction(sharedCancelationFunction);

NSURLRequest *request = NSURLRequestFromImageSource(imageSource);
BOOL hasModuleName = [self->_imageLoader respondsToSelector:@selector(loaderModuleNameForRequestUrl:)];
NSString *moduleName = hasModuleName ? [self->_imageLoader loaderModuleNameForRequestUrl:request.URL] : nil;
std::string moduleCString =
std::string([moduleName UTF8String], [moduleName lengthOfBytesUsingEncoding:NSUTF8StringEncoding]);
telemetry->setLoaderModuleName(moduleCString);

/*
* Even if an image is being loaded asynchronously on some other background thread, some other preparation
* work (such as creating an `NSURLRequest` object and some obscure logic inside `RCTImageLoader`) can take a couple
Expand Down

0 comments on commit e37708d

Please sign in to comment.