-
Notifications
You must be signed in to change notification settings - Fork 0
StructuredConfigReader First Slice — Define and Access Variables, Basic Telemetry #4
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
base: main
Are you sure you want to change the base?
Conversation
…on from DevFoundation
…olidate behavior in StructuredConfigReader
- DidAccessVariableBusEvent: posted when a variable is successfully accessed - DidFailToAccessVariableBusEvent: posted when a variable can't be found (.success(nil)) or if an error is raised when querying providers (.failure(error))
- Use the convenience initializer w/ EventBus parameter to get the standard TelemetryAccessReporter - Use the primary initializer to pass an access reporter type directly.
Code Coverage ReportOverall Coverage: 1.95% (3 of 154) DevConfiguration: 1.95% (3 of 154)
|
| public let accessReporter: any AccessReporter | ||
|
|
||
| /// The internal configuration reader that is used to resolve configuration values. | ||
| let reader: ConfigReader |
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.
I think this can be public.
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.
Whats your though here? If they don't want to use our interface all the time, or our interface isn't up to parity with the internal reader, they should be able to get direct access?
I wanted it private to prevent confusion between the reader's interface and ours, but 🤷♂️
| public let value: ConfigContent | ||
|
|
||
| /// The name of the provider that supplied the value. | ||
| public let source: String |
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.
Could probably just call this providerName to match Swift Configuration.
Sources/DevConfiguration/Telemetry/DidFailToAccessVariableBusEvent.swift
Show resolved
Hide resolved
| /// Returns `true` if sensitive types (like String) should be treated as private. | ||
| /// | ||
| /// This is equivalent to `.auto || .private`. | ||
| var isPrivateForSensitiveTypes: Bool { |
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.
Looking at how you use this, I think I’d rather have some extensions on ConfigVariable. Something like:
extension ConfigVariable {
var isSecret: Bool {
return secrecy == .secret
}
}
extension ConfigVariable<String> {
var isSecret: Bool {
return secrecy != .public
}
}
extension ConfigVariable<[String]> {
var isSecret: Bool {
return secrecy != .public
}
}This works correctly in a playground. It would transform your access functions in StructuredConfigReader into:
return try reader.requiredIntArray(
forKey: variable.key,
isSecret: variable.isSecret
)| /// Variable privacy determines how values are handled in telemetry, logging, | ||
| /// and other observability systems. Secret values are redacted or obfuscated | ||
| /// to prevent sensitive information from being exposed. | ||
| public enum VariablePrivacy { |
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.
Should we change this to VariableSecrecy to match Swift Configuration naming?
| /// | ||
| /// The reader never throws. If resolution fails, it returns the variable's fallback value and posts a | ||
| /// `DidFailToAccessVariableBusEvent` to the event bus. | ||
| public final class StructuredConfigReader { |
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.
Perhaps we should rename this ConfigVariableReader, since that’s really what it is.
Also, does it need to be a class vs. a struct? ConfigReader is a struct.
ConfigReaderwith type-safe access, privacy-aware queries, and always-return semantics that return fallback values on error.