Updated Auth0 Telemetry Format #256
Conversation
Updated Tests Added Platform OS, Version
|
@joshcanhelp Can you validate against RFC please. |
There was a problem hiding this comment.
@cocojoe - Format in your PR description looks good, in general (your macOS and iOS values in the after section are swapped). I wonder, though ... since we're using the convention of '"name:version" in env ("swift":"4.x" for example), should this:
"os_version":"12.1","os":"tvOS"
"os":"iOS","os_version":"11.4"
"os":"macOS","os_version":"10.14"
... be:
"tvOS":"12.1"
"iOS":"11.4"
"macOS":"10.14"
... instead? Takes up less space and the query would be about the same (WHERE tvOS vs WHERE os = "tvOS").
| static func generateValue(fromInfo info: [String: String] = Telemetry.versionInformation()) -> String? { | ||
| static func generateEnviroment() -> [String: String] { | ||
| let platform = Telemetry.osInfo() | ||
| let env = [ "swift": Telemetry.swiftVersion(), |
There was a problem hiding this comment.
Everything runs through a lint, unless it picks up, I have no issue.
| Telemetry.NameKey: name, | ||
| Telemetry.VersionKey: version, | ||
| Telemetry.WrappedVersion: info[Telemetry.VersionKey] ?? Telemetry.NoVersion | ||
| Telemetry.EnvironmentKey: env |
There was a problem hiding this comment.
I'd be careful with a var named env and the string "env", can often get confusing to work with both.
There was a problem hiding this comment.
Agree it might, but think it's fine in this instance.
| if let libVersion = info[Telemetry.VersionKey] as? String { | ||
| env[Telemetry.WrappedVersion] = libVersion | ||
| } else { | ||
| env[Telemetry.WrappedVersion] = Telemetry.NoVersion |
There was a problem hiding this comment.
When would this occur?
There was a problem hiding this comment.
When a Library like Lock.swift is used which wraps around Auth0.swift, we want to know it's Lock.Swift but also what's powering it e.g. Auth0.swift
There was a problem hiding this comment.
Sure but when would Telemetry.NoVersion be used? Is that something we want to allow or should we force a version number?
There was a problem hiding this comment.
TBH I don't know, this was existing behaviour. I imagine something has to go very wrong.
| let data = try? JSONSerialization.data(withJSONObject: info, options: []) | ||
| return data?.a0_encodeBase64URLSafe() | ||
| } | ||
|
|
||
| static func swiftVersion() -> String { |
There was a problem hiding this comment.
Why are you converting this? Seems like something you'll need to keep updated over time rather than just passing it as-is or splitting off and returning the major version number.
There was a problem hiding this comment.
As you can't retrieve it dynamically it's done at compile time, so even to do 4.0, 4.1 you need to do individual checks so you would still need to update over time even to get each new version anyway. This way it should be fine until we hit a new major and that is only at most once a year when we get a new Xcode.
|
@joshcanhelp have updated as discussed internally, please re-check the description as I've updated. Thanks |
|
I'm going to pull this down into Lock.swift for a quick check before merging. |
Changes
Telemetry has been updated inline with latest internal RFC data format Guidelines.
Added platform and platform version to the data.
Fixed Swift version data.
Examples (Refined 23/01)
Before Changes
After PR
When used as a core authentication wrapper, for example in Lock.swift the output will be:
The key
corewas previously used, it could be changed to the name of the lib?References
Please note any links that are not publicly accessible.
Testing
Tests updated.
Checklist