Report deployment and storage environment data in the usage report - #5828
Report deployment and storage environment data in the usage report#5828johnsimons wants to merge 1 commit into
Conversation
warwickschroeder
left a comment
warwickschroeder
left a comment
There was a problem hiding this comment.
I think, if you havent already, we should test the probes using real cloud resources to ensure we arent getting false information back. It would be better to get an "Unavailable" or "Unknown" that a false "Azure SQL Database", for example.
|
|
||
| // Cloud SQL is reached either through a host name or through a unix socket directory named | ||
| // after the instance, so neither end of the value is a reliable place to look. | ||
| return normalized.Contains("cloudsql", StringComparison.Ordinal) ? "GoogleCloudSql" : SelfHosted; |
There was a problem hiding this comment.
If the DB query checks in PostgreSqlDatabaseHostingProbe or SqlServerDatabaseHostingProbe fail for any reason; db down, firewall, wrong password, permissions, etc, it will fallback to checking the connection string. The problem with checking the connection string is that it could be a private DNS name, which if they architectured their system correctly, really should be. So we'll be getting "SelfHosted" rather than "Unknown" or "Unavailable".
Maybe in PostgreSqlDatabaseHostingProbe or SqlServerDatabaseHostingProbe, if it couldnt get the info for whatever reason, it might be better to just return "Unavailable" rather than trying via the connection string. Or if we keep the fallback, perhaps add a 3rd value that tells us where the info came from. The probe or the fallback.
| return "AwsRds"; | ||
| } | ||
|
|
||
| return reader.GetBoolean(3) ? "GoogleCloudSql" : HostingFromConnectionString().Hosting; |
There was a problem hiding this comment.
If the probe query succeeded, and we get to here, and its not azure, aws, or google, is this not evidence that its self hosted?
| public async Task<IEnumerable<(string key, string value)>> GetData(CancellationToken cancellationToken = default) | ||
| { | ||
| yield return ("Features.IntegratedServicePulse", settings.EnableIntegratedServicePulse ? "Enabled" : "Disabled"); | ||
| var notificationsSettings = await notificationsDataStore.LoadSettings(cancellationToken); |
There was a problem hiding this comment.
If this throws, we loose all the other telemetry items. Scope the email notification setting to its own try..catch and return "Unavailable" or something if the DB query throws.
| ("Features.ForwardErrorMessages", Toggle(settings.ForwardErrorMessages)), | ||
| ("Features.EmailNotifications", EmailNotifications(notificationsSettings)), | ||
| ("Retention.ErrorHours", Hours(settings.ErrorRetentionPeriod)), | ||
| ("Retention.AuditHours", Hours(settings.AuditRetentionPeriod)), |
There was a problem hiding this comment.
This setting is coming from the error instance. Is it used?
| var dbContext = scope.ServiceProvider.GetRequiredService<ServiceControlDbContext>(); | ||
|
|
||
| await using var command = dbContext.Database.GetDbConnection().CreateCommand(); | ||
| command.CommandText = "SELECT SERVERPROPERTY('EngineEdition'), SERVERPROPERTY('ProductMajorVersion')"; |
There was a problem hiding this comment.
Should SERVERPROPERTY('ProductMajorVersion') be SERVERPROPERTY('ProductVersion')? I dont thing ProductMajorVersion can be used for anything other than SQL Server (e.g. azure sql database would return null)- https://learn.microsoft.com/en-us/sql/t-sql/functions/serverproperty-transact-sql?view=sql-server-ver17#propertyname
| // EngineEdition is the authoritative answer: the server itself reports which Azure service it is, | ||
| // where a host name suffix is only a guess. Anything on-premises falls back to the suffix. | ||
| string HostingFromEngineEdition(DbDataReader reader) => | ||
| reader.IsDBNull(0) ? HostingFromConnectionString().Hosting : Convert.ToInt32(reader.GetValue(0), CultureInfo.InvariantCulture) switch |
There was a problem hiding this comment.
Are we missing some versions? See EngineEdition at https://learn.microsoft.com/en-us/sql/t-sql/functions/serverproperty-transact-sql?view=sql-server-ver17#propertyname
2 participants
The usage report already carries throughput, versions and whether audit and monitoring are in
use. It says nothing about how the instance is actually deployed or what it stores data in, so
questions like "how many customers run in containers", "who has moved off embedded RavenDB" or
"is anyone using S3 body storage with an IAM role" cannot be answered from the reports we receive.
This adds environment data covering the host, the persistence, the body storage and the
instance's configuration posture.
Keys
Host.Model(Container / WindowsService / Console),Host.Orchestrator,Host.OSPlatform,Host.OSVersion,Host.Architecture,Host.RuntimeVersion,Host.ProcessorCount,Host.AvailableMemoryGBPersistence.Type,Persistence.RavenServer(Embedded / External),Persistence.Hosting,Persistence.ServerVersion,Persistence.FullTextSearchPersistence.BodyStorage.Type,Persistence.BodyStorage.Auth,Limits.MaxBodySizeToStoreSecurity.Authentication,Security.RoleBasedAuthorization,Security.Https,Features.MessageEditing,Features.ExternalIntegrationsPublishing,Features.ForwardErrorMessages,Features.EmailNotifications,Retention.ErrorHours,Retention.AuditHours,Retention.EventsHoursKeys are dotted strings, following the
Features.IntegratedServicePulsekey that was alreadythere.
EnvironmentDataTypeis left alone rather than grown, so adding a datum no longer meanschanging a contract.
No customer data
Every value is a fixed enum member, a boolean, a count or a version number. Nothing derived from
a host name, URL, connection string, bucket or container name, managed identity client id, file
path, machine name or endpoint name is emitted.
RuntimeInformation.OSDescriptionisdeliberately not used because on Linux it carries a distro build string that identifies a
specific image. An acceptance test asserts the report carries no value matching the machine name,
and a unit test asserts no S3 bucket name or access key reaches the report.
Cloud database detection
Persistence.Hostingis resolved by asking the engine where it can, and by classifying theconfigured host name where it cannot.
SQL Server answers definitively through
SERVERPROPERTY('EngineEdition'), where 5, 8 and 11 meanAzure SQL Database, Managed Instance and Synapse. PostgreSQL has no equivalent, so the probe
checks for the administrative roles that each managed offering creates and a self-hosted server
does not:
azure_pg_admin,rds_superuserandcloudsqlsuperuser. Both probes run with a fivesecond command timeout and fall back to host suffix matching on any failure, so a report is never
held up by a database that will not answer.
Persistence.ServerVersionis the engine major version only. SQL Server's minor is always zero,and Azure SQL Database reports major 12 regardless of the engine actually running, so
major.minorwould add a digit that carries no information. Patch level, if it is ever wanted,belongs in a separate key sourced from
ProductUpdateLevel.