Make disk persistence capacity configurable #2329
Conversation
...va/com/azure/monitor/opentelemetry/exporter/implementation/localstorage/LocalFileWriter.java
Outdated
Show resolved
Hide resolved
| @@ -304,6 +304,9 @@ public static class PreviewConfiguration { | |||
| // telemetry (they are much smaller so a larger queue size is ok) | |||
| public int metricsExportQueueCapacity = 65536; | |||
|
|
|||
| // disk persistence has a default capacity of 50MB | |||
| public int diskPersistenceMaxSizeMb = 50; | |||
There was a problem hiding this comment.
2.x SDK enforces a limit of 1 GB:
I'm not really sure the reason, can you see if Application Insights .NET SDK has a max capacity that users can configure? if they do, we may want to implement that max too
also, I wonder if we should allow 0, meaning disable disk persistency, which users on read-only filesystems could use (and then we wouldn't need to log warning for those users)
There was a problem hiding this comment.
dotnet is default to 50MB as well. they use 30 seconds as the default sending interval. same as us.. it's not configured. we could ignore value that is less than 50? and document it?
There was a problem hiding this comment.
dotnet is default to 50MB as well
do they allow user to configure this? and if so, do they have a maximum value that the user can set?
There was a problem hiding this comment.
it's fixed. no configuration.
There was a problem hiding this comment.
but wait, is it exposed to customers though? is it available in the public interface?
There was a problem hiding this comment.
Thx, does the .Net SDK impose any limit on how large the user can configure it to be?
Only limit is the maximum value of long itself.
There was a problem hiding this comment.
looks like it's public API. based on the code, it can be any number.
There was a problem hiding this comment.
https://github.com/microsoft/Telemetry-Collection-Spec/pull/83 we can use that pr to propose a solution.
| // send persisted telemetries from local disk every 30 seconds by default. | ||
| // if diskPersistenceMaxSizeMb is greater than 50, it will get changed to 10 seconds. | ||
| long intervalSeconds = diskPersistenceMaxSizeMb > 50 ? 10 : 30; |
There was a problem hiding this comment.
estimating how long this will take to send a large number of batches from disk:
600 characters per telemetry
- 512 telemetry per batch
= ~300kb per batch
1gb of telemetry will be ~3000 batches
so sending 1 batch per 10 seconds, this will take 8 hours
which is not bad
(we can always revisit this detail when spec'ing it out with other langs)
There was a problem hiding this comment.
i can add a todo to revisit this interval.
There was a problem hiding this comment.
@trask We allow (in .NET) users to configure how many parallel sends can occur. (Default is 10). They can increase it to a large number, increasing the throughput heavily, subject to physical limitations of the network itself.
While scenarios like load testing, by increasing the senders, user could send HUGE amounts of data.
| LocalFileCache localFileCache, | ||
| File telemetryFolder, | ||
| LocalStorageStats stats, | ||
| boolean suppressWarnings) { // used to suppress warnings from statsbeat | ||
| this.telemetryFolder = telemetryFolder; | ||
| this.localFileCache = localFileCache; | ||
| this.stats = stats; | ||
| this.diskPersistenceMaxSizeBytes = diskPersistenceMaxSizeMb * 1024L * 1024L; |
There was a problem hiding this comment.
If it would not be an issue in terms of allocation, it may be worth introducing a DiskPersistenceMaxSize object type. It would have advantages:
- A method could be added to the
DiskPersistenceMaxSizeobject type to encapsulate this kind of code:diskPersistenceMaxSizeMb * 1024L * 1024L; - This would remove the need to add comments (for example in
AzureMonitorExporterBuilderfor this PR) - This would strengthen the code checks at compilation
There was a problem hiding this comment.
comment is only for default value. creating an object for a primitive type with super simple arithmetic is not worthy in my opinion.
No description provided.