WeTransfer / Diagnostics

Allow users to easily share Diagnostics with your support team to improve the flow of fixing bugs.
MIT License
933 stars 53 forks source link

Encode Logging for HTML so object descriptions are visible #51

Closed davidsteppenbeck closed 4 years ago

davidsteppenbeck commented 4 years ago

Would be useful to include object description strings in the logs, but it seems they aren't logged when passed as part of the message argument in the static func DiagnosticsLogger.log(message:).

For example, if I try to log self.description where self is of type UIViewController by calling DiagnosticsLogger.log(message: "This is the view controller description: \(self.description).") what I see in the log is something like "2020-01-27 16:00:00 | ViewController.swift:L100 | This is the view controller description: ."

AvdLee commented 4 years ago

@davidsteppenbeck we're exactly logging what you pass into the framework. The String is parsed before it's being logged.

In other words, if you do the same with a simple print statement it should result in the same.

print("This is the view controller description: \(self.description).")

Are you sure the description contains a value for the view controller?

davidsteppenbeck commented 4 years ago

@AvdLee Yeah, the description contains a value... if I print it in the console, there is info present. I just used a view controller as an example, but this was happening for all object descriptions that I tried.

AvdLee commented 4 years ago

@davidsteppenbeck I'm not sure whether I understand correctly. Would you mind opening a PR for this to fix it based on the reproduction path you have? It sounds like an easy fix!

davidsteppenbeck commented 4 years ago

@AvdLee I guess I didn't explain it very well then!

So, probably better to just paste the results here... Using the simple view controller example, if I have this code:

override func viewDidLoad() {
        super.viewDidLoad()
        let description: String = "viewDidLoad: description = \(self.description)"
        print(description)
        DiagnosticsLogger.log(message: description)
}

Then what I see printed in the console is this: viewDidLoad: description = <VisionCamera.ViewController: 0x104a54200>

But what I see in the report (generated using DiagnosticsReporter.create()) is this: Logs 2020-02-21 17:50:38 System: iOS 13.3.1 Locale: en-US Version: 1.0 (1)

2020-02-21 17:50:38 | ViewController.swift:L19 | viewDidLoad: description = SYSTEM: viewDidLoad: description =


Is this the expected outcome? Perhaps the description is expected to be filtered out in the log? As I said, this is just an example - I really wanted to use this to log the description of an AVCaptureDevice instance to get info about the device camera, but it came up "blank" in the same way.

This isn't really urgent though, so I don't mind trying to figure it out sometime and I'll open a PR when I get it sorted.

AvdLee commented 4 years ago

@davidsteppenbeck could you share the Diagnostics report here? I suspect it from being hidden as an HTML element because of the <> 🤔

davidsteppenbeck commented 4 years ago

Ah, yes, that's it. Changing the file extension to .txt displays the entire logged string as expected. Cheers!

AvdLee commented 4 years ago

It does still mean that we need to fix this. We should probably encode it so it will not be hidden anymore. Great catch! I'll update the title of this issue to reflect that.

davidsteppenbeck commented 4 years ago

Sure! I'm happy to take a stab at this, but perhaps you could first advise what you would like to do here: Would encoding just the <> characters be sufficient or would any other characters require encoding, too?

AvdLee commented 4 years ago

That would be great!

I would love to make this as performant as possible. Encoding every time we add a log might be expensive, especially if the log is eventually not even used if the user is not sending a report.

Instead, maybe we can encode the logs at the moment of creating the report. In that way, we can decide to encode everything for HTML so not only the <> characters.

Does that make sense?

davidsteppenbeck commented 4 years ago

Sure, that makes sense...

However, my knowledge of HTML is minimal, so you might need to guide me a little here - when you say "everything" does that mean really everything (inc. A-Z) or just all the special characters?

AvdLee commented 4 years ago

when you say "everything" does that mean really everything (inc. A-Z) or just all the special characters?

Ah, yeah, that wasn't really clear! I basically meant to encode the complete string for HTML. The encoding method will take care of which symbols it has to encode.

However, as there is no real dedicated method for encoding HTML, we might want to start with simply doing:

extension String {

    /// Encodes entities to be displayed correctly in the final HTML report.
    func HTMLEncoded() -> String {
        replacingOccurrences(of: "<", with: "&lt;").replacingOccurrences(of: ">", with: "&gt;")
    }
}
davidsteppenbeck commented 4 years ago

OK, gotcha... my only confusion here was which symbols specifically to apply replacingOccurrences(of:with:) to. Will start with just < and > as you suggested.

ghost commented 4 years ago

The pull request #58 that closed this issue was merged and released as part of Release 1.6.0 :rocket: Please let us know if the functionality works as expected as a reply here. If it does not, please open a new issue. Thanks!