Issue 974002: Add -[BreakpadController setUploadServerParameters:] for iOS. (Closed)
DescriptionAdd -[BreakpadController setUploadServerParameters:] for iOS. This provides the ability to add server parameters to a crash report when the report is uploaded. BUG=558 Patch Set 1 : dealloc #
Total comments: 8
Patch Set 2 : addUploadParameter:forKey:applyAtUploadTime: #
Total comments: 2
Patch Set 3 : remove NSAssert(!started_, ...) #
Total comments: 3
Patch Set 4 : Breakpad upload_next_report_server_parameters_ #
Total comments: 1
Patch Set 5 : revert to Patch Set 1 #Patch Set 6 : address blundell's comments #
Total comments: 2
Patch Set 7 : fix resetConfiguration #
MessagesTotal messages: 21
How does this relate to BreakpadAddUploadParameter()?
Sign in to reply to this message.
On 2014/01/06 13:04:05, blundell wrote: > How does this relate to BreakpadAddUploadParameter()? BreakpadAddUploadParameter adds parameters when a report is generated. Basically, it modifies the configuration that a crash report sees when it's generated. I want to add a parameter when a previously generated report is uploaded.
Sign in to reply to this message.
https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.h File client/ios/BreakpadController.h (right): https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.... client/ios/BreakpadController.h:92: - (void)setUploadServerParameters:(NSDictionary*)uploadServerParameters; If I'm following you correctly, then I think that this API would be more clear and consistent as something like // Specify a parameter that will be added to crash reports. If |applyAtUploadTime| is YES, the parameter will be added at the time that the crash report is uploaded rather than at the time that the crash report is generated. - (void)addUploadParameter:(NSString*)value forKey:(NSString*)key applyAtUploadTime:(BOOL)applyAtUploadTime; I would then change the method below it to say "Specify a parameter that will be added to crash reports at crash generation time." You could then extend removeUploadParameterForKey to look for it in both dictionaries. Does that make sense, or is there something I'm missing?
Sign in to reply to this message.
PTAL https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.h File client/ios/BreakpadController.h (right): https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.... client/ios/BreakpadController.h:92: - (void)setUploadServerParameters:(NSDictionary*)uploadServerParameters; On the face of it, the API that you suggest is more clear. However, the way that the method interacts with started_ (see the .mm) is inconsistent with the rest of the API. I think it might be better to have different methods for different valid call times. On 2014/01/07 12:56:58, blundell wrote: > If I'm following you correctly, then I think that this API would be more clear > and consistent as something like > > // Specify a parameter that will be added to crash reports. If > |applyAtUploadTime| is YES, the parameter will be added at the time that the > crash report is uploaded rather than at the time that the crash report is > generated. > - (void)addUploadParameter:(NSString*)value forKey:(NSString*)key > applyAtUploadTime:(BOOL)applyAtUploadTime; > > I would then change the method below it to say "Specify a parameter that will be > added to crash reports at crash generation time." > > You could then extend removeUploadParameterForKey to look for it in both > dictionaries. > > Does that make sense, or is there something I'm missing?
Sign in to reply to this message.
https://breakpad.appspot.com/974002/diff/80001/client/ios/BreakpadController.mm File client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/974002/diff/80001/client/ios/BreakpadController.... client/ios/BreakpadController.mm:219: NSAssert(!started_, @"The controller must not be started when " Sorry, I'm a little slow here perhaps: Why should the requirement here be opposite the one below? The reason that others (e.g., setUploadingURL) have the !started assertion is because they get added to the configuration that is passed BreakpadCreate(). That's not true for uploadServerParameters_.
Sign in to reply to this message.
PTAL https://breakpad.appspot.com/974002/diff/80001/client/ios/BreakpadController.mm File client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/974002/diff/80001/client/ios/BreakpadController.... client/ios/BreakpadController.mm:219: NSAssert(!started_, @"The controller must not be started when " Thank you. I did not realize that the !started_ tests were to enforce configuration before create. I thought they came from concerns about multi-threading. On 2014/01/08 17:12:17, blundell wrote: > Sorry, I'm a little slow here perhaps: Why should the requirement here be > opposite the one below? The reason that others (e.g., setUploadingURL) have the > !started assertion is because they get added to the configuration that is passed > BreakpadCreate(). That's not true for uploadServerParameters_.
Sign in to reply to this message.
Thanks for the work. https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController.h File client/ios/BreakpadController.h (right): https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController... client/ios/BreakpadController.h:99: // Specify a parameter that will be added to reports at the time that the crash to crash reports https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController.mm File client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController... client/ios/BreakpadController.mm:219: [uploadServerParameters_ setObject:value forKey:key]; Sorry, now that I see this clearly I have a comment that would require some work :(. Why doesn't Breakpad (i.e., breakpadRef_) maintain this dictionary just like it maintains the generation-time dictionary? Then you could just add an |applyAtUploadTime| optional parameter to BreakpadAddUploadParameter and you could eliminate the addition of the extra argument to BreakpadUploadNextReport. What do you think?
Sign in to reply to this message.
https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController.mm File client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController... client/ios/BreakpadController.mm:219: [uploadServerParameters_ setObject:value forKey:key]; I think that having a server_parameters argument on BreakpadUploadNextReport is consistent with the server_parameters arguments on BreakpadUploadData and BreakpadGenerateReport (particularly BreakpadUploadData). To be honest, I think that trying to integrate upload-time server parameters with crash-time server parameters makes the implementation significantly more opaque. The method bodies in BreakpadController and Breakpad are generally very simple and straightforward. Adding an |applyAtUploadTime| argument reduces the simplicity and clarity of the modified method. So, all in all, I prefer the first patch that I uploaded. On 2014/01/08 20:35:35, blundell wrote: > Sorry, now that I see this clearly I have a comment that would require some work > :(. Why doesn't Breakpad (i.e., breakpadRef_) maintain this dictionary just like > it maintains the generation-time dictionary? Then you could just add an > |applyAtUploadTime| optional parameter to BreakpadAddUploadParameter and you > could eliminate the addition of the extra argument to BreakpadUploadNextReport. > What do you think?
Sign in to reply to this message.
The problem I have with your first patch is twofold: 1) The API's aren't parallel (SetServerUploadParameters vs. AddUploadParameter). 2) As a client of BreakpadController.h, I would have no idea what the difference between SetServerUploadParameters and AddUploadParameter is (exacerbated if the API's were parallel - AddServerUploadParameter vs. AddUploadParameter). Basically, those names aren't expressing the meaningful difference between the two methods. They're all server upload parameters - it's the time at which they're applied that's different. You could do something like AddUploadTimeUploadParameter, but that looks a little silly to me. Once the API's are parallel I would expect the implementations to be parallel as well - i.e., if Breakpad.* keeps one dictionary, why shouldn't it keep the other dictionary? I would optimize for clarity of API and consistency of implementation. Sorry to be slowing you down here...we can VC if that would help move things along the fastest. On 2014/01/08 21:50:51, kiyun wrote: > https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController.mm > File client/ios/BreakpadController.mm (right): > > https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController... > client/ios/BreakpadController.mm:219: [uploadServerParameters_ setObject:value > forKey:key]; > I think that having a server_parameters argument on BreakpadUploadNextReport is > consistent with the server_parameters arguments on BreakpadUploadData and > BreakpadGenerateReport (particularly BreakpadUploadData). > > To be honest, I think that trying to integrate upload-time server parameters > with crash-time server parameters makes the implementation significantly more > opaque. The method bodies in BreakpadController and Breakpad are generally very > simple and straightforward. Adding an |applyAtUploadTime| argument reduces the > simplicity and clarity of the modified method. So, all in all, I prefer the > first patch that I uploaded. > > On 2014/01/08 20:35:35, blundell wrote: > > Sorry, now that I see this clearly I have a comment that would require some > work > > :(. Why doesn't Breakpad (i.e., breakpadRef_) maintain this dictionary just > like > > it maintains the generation-time dictionary? Then you could just add an > > |applyAtUploadTime| optional parameter to BreakpadAddUploadParameter and you > > could eliminate the addition of the extra argument to > BreakpadUploadNextReport. > > What do you think?
Sign in to reply to this message.
On 2014/01/09 16:56:57, blundell wrote: > The problem I have with your first patch is twofold: > > 1) The API's aren't parallel (SetServerUploadParameters vs. AddUploadParameter). > 2) As a client of BreakpadController.h, I would have no idea what the difference > between SetServerUploadParameters and AddUploadParameter is (exacerbated if the > API's were parallel - AddServerUploadParameter vs. AddUploadParameter). > > Basically, those names aren't expressing the meaningful difference between the > two methods. They're all server upload parameters - it's the time at which > they're applied that's different. You could do something like > AddUploadTimeUploadParameter, but that looks a little silly to me. > > Once the API's are parallel I would expect the implementations to be parallel as > well - i.e., if Breakpad.* keeps one dictionary, why shouldn't it keep the other > dictionary? > > I would optimize for clarity of API and consistency of implementation. My point is that I don't feel that we are optimizing for clarity of API and consistency of implementation. Passing server_parameters to BreakpadUploadData and BreakpadGenerateReport already exists and what I intend to do with BreakpadUploadNextReport is quite consistent with how those functions handle server_parameters. Furthermore, I don't think there's any reason for Breakpad to track anything that can be passed in at upload time, because Breakpad's callers determine when uploads happen. In contrast, BreakpadController's callers don't have the same amount of control over uploading, so it's more appropriate for BreakpadController to provide setters for upload configuration (which it does for the upload interval and upload URL). > > Sorry to be slowing you down here...we can VC if that would help move things > along the fastest. > > On 2014/01/08 21:50:51, kiyun wrote: > > > https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController.mm > > File client/ios/BreakpadController.mm (right): > > > > > https://breakpad.appspot.com/974002/diff/120001/client/ios/BreakpadController... > > client/ios/BreakpadController.mm:219: [uploadServerParameters_ setObject:value > > forKey:key]; > > I think that having a server_parameters argument on BreakpadUploadNextReport > is > > consistent with the server_parameters arguments on BreakpadUploadData and > > BreakpadGenerateReport (particularly BreakpadUploadData). > > > > To be honest, I think that trying to integrate upload-time server parameters > > with crash-time server parameters makes the implementation significantly more > > opaque. The method bodies in BreakpadController and Breakpad are generally > very > > simple and straightforward. Adding an |applyAtUploadTime| argument reduces the > > simplicity and clarity of the modified method. So, all in all, I prefer the > > first patch that I uploaded. > > > > On 2014/01/08 20:35:35, blundell wrote: > > > Sorry, now that I see this clearly I have a comment that would require some > > work > > > :(. Why doesn't Breakpad (i.e., breakpadRef_) maintain this dictionary just > > like > > > it maintains the generation-time dictionary? Then you could just add an > > > |applyAtUploadTime| optional parameter to BreakpadAddUploadParameter and you > > > could eliminate the addition of the extra argument to > > BreakpadUploadNextReport. > > > What do you think?
Sign in to reply to this message.
PTAL Patch Set 4 contains the version with the upload server parameters in Breakpad.
Sign in to reply to this message.
Thanks for the explanation. I understand the motivation behind Patch Set 1 much better now: whereas I had been thinking about this new API as parallel to AddUploadParameter, you had been thinking about it as parallel to the existing Set* methods of BreakpadController. I can see the logic in that. I would be OK moving back to Patch Set 1 with an improved name for the setter (SetAdditionalUploadTimeParameters?), except that we need to resolve the issue that I noticed on Patch Set 4 first. If we determine that the prefix *should* be added for these parameters (which my default assumption is that it should), then I think it will make sense to stay with Patch Set 4. Thanks for the work and discussion! https://breakpad.appspot.com/974002/diff/90008/client/ios/Breakpad.mm File client/ios/Breakpad.mm (right): https://breakpad.appspot.com/974002/diff/90008/client/ios/Breakpad.mm#newcode742 client/ios/Breakpad.mm:742: if (applyAtUploadTime) { It looks like you deliberately chose not to add the prefix for the upload-time parameters. What is the reasoning for that? I would think that you would want these parameters treated identically to those that the client of Breakpad added to be applied at crash generation time. Note that this question applies to Patch Set 1 as well.
Sign in to reply to this message.
On 2014/01/09 20:54:13, blundell wrote: > Thanks for the explanation. I understand the motivation behind Patch Set 1 much > better now: whereas I had been thinking about this new API as parallel to > AddUploadParameter, you had been thinking about it as parallel to the existing > Set* methods of BreakpadController. I can see the logic in that. I would be OK > moving back to Patch Set 1 with an improved name for the setter > (SetAdditionalUploadTimeParameters?), except that we need to resolve the issue > that I noticed on Patch Set 4 first. If we determine that the prefix *should* be > added for these parameters (which my default assumption is that it should), then > I think it will make sense to stay with Patch Set 4. > > Thanks for the work and discussion! > > https://breakpad.appspot.com/974002/diff/90008/client/ios/Breakpad.mm > File client/ios/Breakpad.mm (right): > > https://breakpad.appspot.com/974002/diff/90008/client/ios/Breakpad.mm#newcode742 > client/ios/Breakpad.mm:742: if (applyAtUploadTime) { > It looks like you deliberately chose not to add the prefix for the upload-time > parameters. What is the reasoning for that? I would think that you would want > these parameters treated identically to those that the client of Breakpad added > to be applied at crash generation time. Note that this question applies to Patch > Set 1 as well. If you look at translateConfigurationData: in client/mac/sender/uploader.mm, you'll see that the prefix is stripped off of the config keys before being passed to Uploader's addServerParameter:forKey:, which is the method that I call from Breakpad::UploadNextReport(). So, there's no need to store them with the prefix.
Sign in to reply to this message.
OK, I'm on board now. Thanks! Patch Set 1 LGTM with nits. Apologies for the runaround. https://breakpad.appspot.com/974002/diff/20001/client/ios/Breakpad.mm File client/ios/Breakpad.mm (right): https://breakpad.appspot.com/974002/diff/20001/client/ios/Breakpad.mm#newcode461 client/ios/Breakpad.mm:461: for (NSString *key in server_parameters) { Does this not do the right thing (exit out of the loop without doing any iterations) if server_parameters is nil? https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.h File client/ios/BreakpadController.h (right): https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.... client/ios/BreakpadController.h:92: - (void)setUploadServerParameters:(NSDictionary*)uploadServerParameters; I would call this -setParametersToAddAtUploadTime:uploadTimeParameters. https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.... client/ios/BreakpadController.h:94: // Specify a parameter that will be uploaded to the crash server. See I would change this comment to something like "Specify a parameter that will be added to the crash report when a crash is generated."
Sign in to reply to this message.
PTAL https://breakpad.appspot.com/974002/diff/20001/client/ios/Breakpad.mm File client/ios/Breakpad.mm (right): https://breakpad.appspot.com/974002/diff/20001/client/ios/Breakpad.mm#newcode461 client/ios/Breakpad.mm:461: for (NSString *key in server_parameters) { On 2014/01/09 21:11:41, blundell wrote: > Does this not do the right thing (exit out of the loop without doing any > iterations) if server_parameters is nil? Done. I was being conservative. https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.h File client/ios/BreakpadController.h (right): https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.... client/ios/BreakpadController.h:92: - (void)setUploadServerParameters:(NSDictionary*)uploadServerParameters; On 2014/01/09 21:11:41, blundell wrote: > I would call this -setParametersToAddAtUploadTime:uploadTimeParameters. Done. https://breakpad.appspot.com/974002/diff/20001/client/ios/BreakpadController.... client/ios/BreakpadController.h:94: // Specify a parameter that will be uploaded to the crash server. See On 2014/01/09 21:11:41, blundell wrote: > I would change this comment to something like "Specify a parameter that will be > added to the crash report when a crash is generated." Done.
Sign in to reply to this message.
If it looks okay to you, can you please land it for me?
Sign in to reply to this message.
https://breakpad.appspot.com/974002/diff/160012/client/ios/BreakpadController.mm File client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/974002/diff/160012/client/ios/BreakpadController... client/ios/BreakpadController.mm:196: [self setUploadServerParameters:nil]; You need to change this callsite.
Sign in to reply to this message.
PTAL. If you're happy with it, can you please land it for me? https://breakpad.appspot.com/974002/diff/160012/client/ios/BreakpadController.mm File client/ios/BreakpadController.mm (right): https://breakpad.appspot.com/974002/diff/160012/client/ios/BreakpadController... client/ios/BreakpadController.mm:196: [self setUploadServerParameters:nil]; On 2014/01/10 14:57:18, blundell wrote: > You need to change this callsite. Done. Sorry about that. I didn't realize that the Breakpad Xcode project has unknown methods configured as warnings, not errors. I reviewed all the warnings and made sure that this CL does not add any new ones.
Sign in to reply to this message.
Committed as r1271.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
