-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make disk persistence capacity configurable #2329
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,24 +34,25 @@ | |
/** This class manages writing a list of {@link ByteBuffer} to the file system. */ | ||
final class LocalFileWriter { | ||
|
||
// 50MB per folder for all apps. | ||
private static final long MAX_FILE_SIZE_IN_BYTES = 52428800; // 50MB | ||
private static final String PERMANENT_FILE_EXTENSION = ".trn"; | ||
|
||
private final long diskPersistenceMaxSizeBytes; | ||
private final LocalFileCache localFileCache; | ||
private final File telemetryFolder; | ||
private final LocalStorageStats stats; | ||
|
||
private final OperationLogger operationLogger; | ||
|
||
LocalFileWriter( | ||
int diskPersistenceMaxSizeMb, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it would not be an issue in terms of allocation, it may be worth introducing a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment is only for default value. creating an object for a primitive type with super simple arithmetic is not worthy in my opinion. |
||
|
||
operationLogger = | ||
suppressWarnings | ||
|
@@ -63,7 +64,7 @@ final class LocalFileWriter { | |
|
||
void writeToDisk(String instrumentationKey, List<ByteBuffer> buffers) { | ||
long size = getTotalSizeOfPersistedFiles(telemetryFolder); | ||
if (size >= MAX_FILE_SIZE_IN_BYTES) { | ||
if (size >= diskPersistenceMaxSizeBytes) { | ||
operationLogger.recordFailure( | ||
"Local persistent storage capacity has been reached. It's currently at (" | ||
+ (size / 1024) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ public class LocalStorageTelemetryPipelineListener implements TelemetryPipelineL | |
|
||
// telemetryFolder must already exist and be writable | ||
public LocalStorageTelemetryPipelineListener( | ||
int diskPersistenceMaxSizeMb, | ||
File telemetryFolder, | ||
TelemetryPipeline pipeline, | ||
LocalStorageStats stats, | ||
|
@@ -48,9 +49,14 @@ public LocalStorageTelemetryPipelineListener( | |
LocalFileCache localFileCache = new LocalFileCache(telemetryFolder); | ||
LocalFileLoader loader = | ||
new LocalFileLoader(localFileCache, telemetryFolder, stats, suppressWarnings); | ||
localFileWriter = new LocalFileWriter(localFileCache, telemetryFolder, stats, suppressWarnings); | ||
localFileWriter = | ||
new LocalFileWriter( | ||
diskPersistenceMaxSizeMb, localFileCache, telemetryFolder, stats, suppressWarnings); | ||
|
||
localFileSender = new LocalFileSender(loader, pipeline, suppressWarnings); | ||
// 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; | ||
Comment on lines
+56
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. estimating how long this will take to send a large number of batches from disk: 600 characters per telemetry
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i can add a todo to revisit this interval. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
localFileSender = new LocalFileSender(intervalSeconds, loader, pipeline, suppressWarnings); | ||
localFilePurger = new LocalFilePurger(telemetryFolder, suppressWarnings); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.x SDK enforces a limit of 1 GB:
ApplicationInsights-Java/core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionFileSystemOutput.java
Line 83 in d3a73be
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dotnet source code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fixed. no configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but wait, is it exposed to customers though? is it available in the public interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only limit is the maximum value of
long
itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it's public API. based on the code, it can be any number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thx all 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/microsoft/Telemetry-Collection-Spec/pull/83 we can use that pr to propose a solution.