Skip to content
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

Expose diagnostic metadata properties at top level #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amonshiz
Copy link

@amonshiz amonshiz commented Feb 2, 2023

The interesting bits of data for each diagnostic type are on the metadata types. However those properties were also private so not accessible to users of Meter. Let's expose them as public but also add convenience accessors at the diagnostic type level.

I also changed the bundling of the test resources because .copy was introducing a level of indirection/nesting in the final bundle that caused the tests to fail in Xcode 14.2. With the .copy and the extra level of nesting the paths in the tests would need to change, and given there were no resources with the same name being merged into the bundle that seemed unnecessary.

Before After
CleanShot 2023-02-02 at 15 48 25@2x CleanShot 2023-02-02 at 15 49 45@2x

Addresses #7

…r and change resource bundling to process to not introduce a level of indirection on disk
@mattmassicotte
Copy link
Contributor

Thank you so much for putting this together! I know I've been a little negligent keeping up with the types I don't actually use myself as much.

Can you help me understand why making them public on the meta data types is helpful? That was really just an implementation detail to deal with the API/encoding mismatch. Now these structures no longer match the corresponding classes from MetricKit. I'm open to it! I just want to understand.

@mattmassicotte
Copy link
Contributor

Also, what does the change from copy->process do?

@amonshiz
Copy link
Author

amonshiz commented Feb 2, 2023

Can you help me understand why making them public on the meta data types is helpful? That was really just an implementation detail to deal with the API/encoding mismatch. Now these structures no longer match the corresponding classes from MetricKit. I'm open to it! I just want to understand.

I'm sorry, I'm lost. XD

I read your comment on #7 as recommending to expose the metadata type properties at the diagnostic type level. But I can see I misunderstood and failed to actually convert to the expected type. Will do!

In truth I was going to use them as strings in our code anyway so I just worked for me to leave them that way :).

Also, what does the change from copy->process do?

Updated the PR description with screenshots of the file layout before and after. Before == .copy and After == .process. However, I was running in Xcode and not the command line for testing. When run at the command line with $ swift test there is no difference in the resource bundle structure. So ... that's fun.

Process rule
For most use cases, use process(_:localization:) to apply this rule and have Xcode process the resource according to the platform you’re building the package for. For example, Xcode may optimize image files for a platform that supports such optimizations. If you apply the process rule to a directory’s path, Xcode applies the rule recursively to the directory’s contents. If no special processing is available for a resource, Xcode copies the resource to the resource bundle’s top-level directory.

Copy rule
Some Swift packages may require a resource file to remain untouched or to retain a certain directory structure for resources. Use the copy(_:) function to apply this rule and have Xcode copy the resource as is to the top level of the resource bundle. If you pass a directory path to the copy rule, Xcode retains the directory’s structure.

https://developer.apple.com/documentation/xcode/bundling-resources-with-a-swift-package

@mattmassicotte
Copy link
Contributor

You know, I always wondered why these didn't work within Xcode. That's awesome!!

Totally my fault for not being more clear. The specialized metadata types are actually there only to help out with serialization. MetricKit doesn't expose them - there's only a generic MXMetaData data. By making these public on the metadata types, we're adding API that doesn't match MetricKit. All your accesses should go through the diagnostics themselves. That's why I was trying to keep a more clear distinction between what's public and private there.

This is very minor, and honestly might be more annoying that helpful. I was just trying to keep as close as possible to the actual MetricKit types.

@amonshiz
Copy link
Author

amonshiz commented Feb 2, 2023

Totally my fault for not being more clear. The specialized metadata types are actually there only to help out with serialization. MetricKit doesn't expose them - there's only a generic MXMetaData data. By making these public on the metadata types, we're adding API that doesn't match MetricKit. All your accesses should go through the diagnostics themselves. That's why I was trying to keep a more clear distinction between what's public and private there.

This is very minor, and honestly might be more annoying that helpful. I was just trying to keep as close as possible to the actual MetricKit types.

No no. I like the idea for sure! I am finding that converting from the JSON representation in the test resources back to Measurement<UnitDuration> is "interesting". In the test resources a duration looks like

{
...
  "hangDuration": "20 sec"
...
}

But if you were to serialize a Measurement<UnitDuration>(value: 20, unit: .seconds) you would get something like (in the Swift REPL):

String(data: JSONEncoder().encode(Measurement(value: 20, unit: UnitDuration.seconds)), encoding: .utf8)
$R3: String? = "{\"value\":20,\"unit\":{\"converter\":{\"constant\":0,\"coefficient\":1},\"symbol\":\"s\"}}"

I know there is MeasurementFormatter for going Measurement -> String but I cannot find the effective inverse (unlike regular old NumberFormatter or the various data detectors). An option is to split on spaces and take the first and trust it will always be a value of seconds. That is kind of sane given the following out put of MeasurementFormatter when asked to create a string for 20hours:

MeasurementFormatter().string(from: Measurement(value: 20, unit: UnitDuration.hours))
$R5: String = "72,000 sec"

Meaning something like below kind of works (though would not commit it with !s :) ).

let numberFormatter = NumberFormatter()
numberFormatter.numberStyle = .decimal
numberFormatter.number(from: hangDuration.split(" ").first!)!.doubleValue

@mattmassicotte
Copy link
Contributor

Riiiight. Now I'm remembering why I didn't do this in the first place... Honestly, MetricKit drives me bananas! It converts so much useful structure into strings.

I'm not opposed to hacking in some parsing.

You are consuming these on-device, right? Can you consume the real MetricKit payloads? I know this does not solve the immediate issue here, but I'm just curious.

@amonshiz
Copy link
Author

amonshiz commented Feb 3, 2023

Riiiight. Now I'm remembering why I didn't do this in the first place... Honestly, MetricKit drives me bananas! It converts so much useful structure into strings.

I'm not opposed to hacking in some parsing.

You are consuming these on-device, right? Can you consume the real MetricKit payloads? I know this does not solve the immediate issue here, but I'm just curious.

We want to land this code before we have dropped support for BigSur. And so that leaves me with essentially reimplementing Meter to deal with availability on BigSur. I really liked how clean your implementation was and only was caught up when trying to access the few special properties.

Our specific needs at this time would be satisfied with the values as strings. That would let us start collecting information from coworkers and then figure out a general pattern to the data and how MetricKit is encoding. Of course we could go the disassembly route to figure it out but I'm not sure that would be any faster for me :).

I will take a pass at the bespoke parsing but I'm not entirely sure how much time I can commit to it. I wonder if there is a way to have the diagnostic types extract the data directly from the MXDiagnostic types. This might work because the various Measurement and Units in use are at least macOS 10.15+ and iOS 13+, it is just the MXDiagnostic types themselves that are in the way (at least for me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants