Repository navigation
Conversation
…into audit-external-bodies
| | `--tag <tag>` | Test a specific ServiceControl image tag, for example a PR prerelease like `pr-1234` | | ||
| | `--persistence <type>` | Persistence for the error instance: `RavenDb` (default), `SqlServer`, or `PostgreSql` | | ||
| | `--error-ingestion-scale-unit <n>` | Start `n` additional error-ingestion-only ServiceControl instances | | ||
| | `--azureblob-body-storage "<connection string>"` | Store message bodies in Azure Blob Storage instead of the container file system. Applies only to `SqlServer` and `PostgreSql`. The error instance and its scale-out units share one container, and each audit instance gets its own | |
There was a problem hiding this comment.
is this missing a "-" between azure and blob?
There was a problem hiding this comment.
I was treating it as a single noun, you are right it might be more "correct" to have a dash.
|
|
||
| External storage is required. The instance fails to start when `MessageBody/StorageType` is missing, which includes existing SQL Server and PostgreSQL Audit instances that upgrade without adding it. Setup creates the directory, container, or bucket. | ||
|
|
||
| Settings use the `ServiceControl.Audit/` prefix in the application settings file. The validation matches the Error instance, whose settings use the `ServiceControl/` prefix. |
There was a problem hiding this comment.
Need to add to docs PR, if not already
| | `MessageBody/MinCompressionSize` | all, minimum body size in bytes to compress | 4096 | | ||
| | `MessageBody/FileSystem/StoragePath` | `FileSystem`, required | none | | ||
| | `MessageBody/FileSystem/DataSpaceRemainingThreshold` | `FileSystem`, free space percentage below which the custom check fails | 15 | | ||
| | `MessageBody/Azure/ConnectionString` | `AzureBlob`, shared key or SAS | none | |
There was a problem hiding this comment.
So we are only supporting authentication via SAS token? What about Managed Identity or other Entra identities?
| @@ -0,0 +1,79 @@ | |||
| # Message body storage | |||
There was a problem hiding this comment.
Any ADRs worth extracting for the message body design?
| var storage = new RecordingBodyStorage(); | ||
| await configuration.CreateRetention(storage).SweepNow(); | ||
|
|
||
| Assert.That(storage.DeletedPrefixes, Has.Some.Matches<string>(bodyId.StartsWith)); |
There was a problem hiding this comment.
Has.Some.Matches(bodyId.StartsWith) also passes for a day prefix, or "2", which would delete every body still in retention.
| return external is null | ||
| ? MessageBodyView.NoContent() | ||
| : MessageBodyView.FromStream(external.Stream, external.ContentType, external.BodySize, version); |
There was a problem hiding this comment.
When the row says External but storage has nothing, this answers 204 with no log. A LogWarning with the body id and row here would make it traceable.
| await foreach (var blob in container.GetBlobsAsync(BlobTraits.None, BlobStates.None, prefix, cancellationToken: cancellationToken)) | ||
| { | ||
| await container.GetBlobClient(blob.Name).DeleteIfExistsAsync(cancellationToken: cancellationToken); | ||
| } |
There was a problem hiding this comment.
This deletes one blob per round trip, in sequence. An hour of audit traffic with binary bodies can be hundreds of thousands of blobs, so the sweep can take as long as the hour it is clearing, and the rows (and on PostgreSQL the partition drop) wait behind it. S3 already deletes 1,000 per call.
BlobBatchClient.DeleteBlobsAsync (256 per batch) would keep Azure in the same range.
| foreach (var filePath in Directory.EnumerateFiles(StoragePath, $"{prefix}*.body")) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| if (Path.GetFileName(filePath).StartsWith(prefix, StringComparison.Ordinal)) | ||
| { | ||
| TryDelete(filePath); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bodies are one flat directory, and this enumeration reads every file in it to find one hour's worth. With a month of audit bodies that's tens of millions of entries per hourly sweep.
Would per-hour subdirectories ({StoragePath}/{yyyyMMddHH}/{guid}.body) work for audit, so the delete becomes Directory.Delete?
There was a problem hiding this comment.
Exactly, here is something I prepared earlier - https://github.com/Particular/ServiceControl/pull/5318/changes#diff-4d7b8cefc9a08fc2d1c1480d29b16e47cbf4d4c926dcbb4b6f0c54a3355c0c26R162-R181
| catch (RequestFailedException ex) when (ex.Status is (int)HttpStatusCode.Conflict or (int)HttpStatusCode.PreconditionFailed) | ||
| { | ||
| // A blob for this immutable body already exists. | ||
| } |
There was a problem hiding this comment.
This will swallow every 409 and 412 as "already exists". 409 is also ContainerBeingDeleted and ContainerDisabled, so writes during a container recreate succeed silently and audit rows point at bodies that were never stored.
Filtering on ex.ErrorCode is BlobErrorCode.BlobAlreadyExists or BlobErrorCode.ConditionNotMet would keep the intended case.
|
So this would be a new feature? |
|
So |
|
|
||
| Ingestion writes external bodies before the batch transaction that inserts the rows. A failed write fails the batch, and no row is inserted. A failed insert can leave an unreferenced body. | ||
|
|
||
| A body is stored under its audit body id, `yyyyMMddHH-{unique message id}`, the same id used in the body URL. Storage keeps the first write of an id and ignores later writes. |
There was a problem hiding this comment.
Why is this prefixed with yyyyMMddHH-?
When we store audit message we generate a new uuid, can't we use uuid v7 so that these already contain a timestamp?
Based on the retention section it seems this is done for cleanup based on the retention window?
|
|
||
| The unit differs by provider, so the prefix is part of `IAuditPartitionManager`: | ||
|
|
||
| - SQL Server deletes rows one hour at a time. The prefix is the hour, `yyyyMMddHH-`. |
There was a problem hiding this comment.
Is that internval configurable? Will it to chunks? Is that a native SQL server feature or an application query?
| - SQL Server deletes rows one hour at a time. The prefix is the hour, `yyyyMMddHH-`. | ||
| - PostgreSQL drops whole day partitions without scanning rows. The prefix is the day, `yyyyMMdd`, and it matches every hour of that day. A day is only dropped once all of it is older than the retention period, so no body is removed earlier than its row would be. | ||
|
|
||
| Prefixes must not be empty and must not contain wildcard or path characters. SQL Server only visits hours that have rows, so an unreferenced body in an hour with no rows stays until that location is cleaned manually. PostgreSQL visits every expired day partition, including pre-created partitions without rows, so it also cleans unreferenced bodies of those days. |
There was a problem hiding this comment.
How to the other implementation deal with retention?
|
|
||
| An invalid value, or missing settings for the chosen type, still fails at startup. Setup creates the directory, container, or bucket. | ||
|
|
||
| Audit settings use the `ServiceControl.Audit/` prefix in the application settings file. Provider-specific validation matches the Error instance, whose settings use the `ServiceControl/` prefix. |
There was a problem hiding this comment.
Is it a conscious decision that primary and audit do not share the same body store?
There was a problem hiding this comment.
Yes, the problem with sharing the large message storage is that you can no longer clean up unused bodies without cooperation between the instances that have different retention windows.
The implementation allows them to share the same storage account, but the bodies themselves are not shared.
| <add key="ServiceControl.Audit/MessageBody/S3/KeyPrefix" value="audit-instance-1/" /> | ||
| ``` | ||
|
|
||
| ## One location per instance |
There was a problem hiding this comment.
I miss decisions like body compression and encryption
|
@rbev the delimiter for the prefix needs to be set by the implementation, example for Azure Blobs, we want the prefix to be Have a look at my implementation of this in https://github.com/Particular/ServiceControl/pull/5318/changes#diff-985fb65dab014cdcb60c6d03ef95370d1059107f6887c478fdd2c175f9decab1 and https://github.com/Particular/ServiceControl/pull/5318/changes#diff-44f054b50a09b2ab13d8985465c7db9b57fd103451e40fc3030ba889c28624e7R24. |
Problem
Audit's SQL Server and PostgreSQL persisters discard oversized and binary message bodies. Oversized text retains a searchable prefix, but the body API cannot return the complete body. Error's SQL persisters already support external storage for these bodies.
Change
Reuse Error's FileSystem, Azure Blob, and S3 providers for Audit by extracting them into
ServiceControl.Persistence.BodyStorage.RavenDB behavior is unchanged. Existing
TooLargeandNotTextrows retain their previous API responses; discarded bodies cannot be recovered. The new external-body state uses the existing integer column, so no schema migration is needed.Required configuration
External storage is required for Audit SQL Server and PostgreSQL instances. Existing instances must add
ServiceControl.Audit/MessageBody/StorageTypeand the selected provider's settings before restarting after an upgrade.MaxBodySizeToStorebecomes the inline threshold, not a limit on the complete stored body. Azure defaults to theaudit-bodiescontainer; S3 defaults to theaudit-bodies/key prefix.Each instance must use a separate directory, container, or key prefix. Sharing a location can let one instance's retention process delete another instance's bodies.
Design constraints
Sharing the providers avoids duplicating their storage formats, compression, authentication, and stream handling. Error retains its existing configuration and behavior.
Audit retention deletes by hourly prefixes on SQL Server and daily prefixes on PostgreSQL. This preserves PostgreSQL's partition-drop path without scanning message rows. SQL Server only visits hours containing rows, so bodies orphaned by failed inserts into otherwise empty hours can require manual cleanup.
The current design and configuration are documented in Message body storage.