maybe headers.toMap in the end to make it read-only again?

yup I also had this in mind but I was thinking that might be a bit overkill...I could actually do it differently without having 2 create 2 maps. Let me change this.

Actually the reason why I did this was that I return a immutable map. If you call toMap under the hood it also creates an LinkedHashMap so still mutable.

So basically I will leave it like this because under the hood after a quick research toMap returns like I mentioned above a LinkedHashMap. Actually I would have expected to return an ImmutableMap but apparently the immutability is only at the type (surface level). So in a Kotlin code you could easily do this:

  val map = mapOf(...)
  val immutableMap = map.toMap()
  val hashMap = immutableMap as LinkedHashMap

is this @Supress necessary? it seems you catch all the possible exceptions

good questions...it was before I added this in the detekt.xml let me see it now.

yes it is still complaining and due to the NullPointerException catch at the end of the block. I am not sure why though as this is specifically thrown by the code under the hood.

It's a default set of detekt to include NPE in this check, because for Kotlin code, NPE is not very informative, but since we are calling a Java function, there is nothing we can do about, so it's correct to have @Suppress here I think.

https://detekt.dev/docs/rules/exceptions/#toogenericexceptioncaught

Seems like this sentence should be corrected. It should add at least the comma and state, that this is a code of the previous attempt, otherwise one may think that it can be the code of the previous batch upload.

I am going to try to be more explicit here.

why duplicate the description there if it is already in the Kdoc above? These ones won't be treated as documentation and are ordinary comments.

Also to avoid confusion we must say that it is for the current batch only (otherwise it is not clear if it is for the previous batch as well).

will it be used for any deduplication actually? I was under the impression that this is mostly for collecting the statistics right now

Well yes maybe not for now, not sure should we change this definition ?

I'm not even sure it will ever will. And anyway, if it will be, it will be used only for RUM events.

should add a space in the beginning in case if there is already some text, otherwise it will be like ...targeting the relevant Datadog site.Attempt count was...

So each feature has to provide the key on its own? I'm not sure if it is the solution we would like to have. Is it possible to lift it to the core? Or at least have something like ByteArray.sha1() declared in the core. How iOS SDK is doing that?

I went to the core way but then I've seen that in iOS implementation they did it at the feature level and changed it. I think it makes sense at the feature level as Logs and Traces will not need this. Otherwise in the core you will have to make sure you do it only for the right features (if/else) and not even sure if we have the feature information in that place to check.

They have a function to generate the key in the core https://github.com/DataDog/dd-sdk-ios/blob/dec751b901e17e74bc7f96af56558882cacd9282/DatadogInternal/Sources/Extensions/Data%2BCrypto.swift#L11 and it is applied on the each feature level. We should follow the same approach.

The only problem I have with this is that for now we are doing this only for RUM and I am afraid it will stay like that. I have no problem with moving this into core but that's the reason I preferred not to do it. Should we do it now or later in case we really need to share it between features ?

I mean for now we can just declare extension method to generate SHA1 in the core and call it from the feature-specific factory. We already have the header declared in the core, isn't it?

I get the part with the function to have the extension in the core but don't get the header part ? The header is added here so you only need ByteArray.generateSha1Fingerprint() function if we want to go this way. I am still thinking it is overkilled as no other code will ever use this function but will let it here for others to say their opinion. @ambushwork @xgouchet @jonathanmos ?

While I agree this could be done in different ways the initial plan for me was to align to iOS implementation where this was added to the UploadStatus information, I also kept their naming -> attempt.

yup here is a corner case and having followed the exact implementation in iOS I think they have the same corner case. I am going to open the discussion in the daily today to make sure everyone is aware.

We have 2 options here, either keep the information in the UploadStatus and handle the previousUpload status by its batchID or move everything into the DataOkHttpUploader but I would rather not do it there as we do not have any information about the batch id and we will have to compute the fingerprint from the batchData or pass the batchID as a extra parameter to that method.

the answer to this corner this depends on implementation on when you are reading the content from the file, for iOS it is not a problem because the batch is already in the memory and then retry state management is done. If you delete the files, it doesn't matter, we have the data in memory already read in case of iOS - this is by design and I would even say it is bad design and can cause other issues but not with retry information.

actually no - scratch above, we don't keep in memory. the deletion of batch for other than successful upload can be a problem here.

I would suggest save the batch identifier in the upload status just like retry information and only propagate the information when the identifier is same - else reset it.

I don't see any reason to keep this per batch, only for the active batch is sufficient.

I agree with this solution, this will mean that we can keep same implementation and just pass the batchID in the previous UploadStatus to match it here.

Yes I agree, I need to add more info here as this is quite confusing. Basically to answer to your question, it should contain also the current one to be consistent but in that case the previousStatusCode will be null. In the RequestFactory we will not send this information if the previousStatusCode is null

Here's a proposal to make this property more understandable at a glance.

yup I realized that, I am fixing it now, I am afraid I will have to do this in function of the target.

Here's a proposal to make this property more understandable at a glance.

This potentially may be not a thread-safe, leading to the wrong data read. The thing is that here

Now we share this executor with particular features which submit runnables to this executor and while for the feature all its runnables will be executed in the sequential way, there is a low chance that under the high load it may be more than 1 thread in this executor at runnable N can be executed by other thread than runnable N-1.

I may be wrong here, but on the quick look it is a concern.

I particularly looked into this myself also. Initially my intention was to make them thread safe but then I didn't want to add this if it's not really needed. I checked the executors and seen the 1 pool thread size and assumed it is fine. I am thinking that if we really have this scenario that you are mentioning we might end up in different problems as the upload runnable assumes that everything is executed sequentially.

if we really have this scenario that you are mentioning we might end up in different problems as the upload runnable assumes that everything is executed sequentially

And it will be executed sequentially (in the boundaries of the particular feature) even if there are multiple threads in the pool, because there is only one single runnable at the time for the particular feature and the next runnable is scheduled only at the end of the execution of the previous feature. And underlying storage is thread-safe, it is designed for the concurrent usage.

Here my point is only about volatile nature of these references. Imagine there is an executor running and thread was killed because of some reason; then new thread will be spinned off which may not see the changes made to these references by the previous thread. Or in the case if executor has multiple threads running.

That might be a problem indeed. I think marking them as volatile might make sense to make sure they are being kept in the HEAP and not in the Thread local memory. In case of atomicity we don't need it in this case as we are already running sequential. I am going to open a PR.