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

Introduce #dsoHandle and args in addition to #file, #function and #line #72

Open
shpakovski opened this issue May 6, 2019 · 3 comments

Comments

@shpakovski
Copy link

shpakovski commented May 6, 2019

Hello,

This is more like a pitch, not an issue at all, and it’s probably too late. However, I’d like to have a custom handler for os_log, something like this works fine for debugging purposes, even though it’s using private API:

import os
import _SwiftOSOverlayShims

...

func log(
  level: Logger.Level, message: Logger.Message, metadata: Logger.Metadata?,
  file: String, function: String, line: UInt, dso: UnsafeRawPointer, args: CVarArg...)
{
  let ra = _swift_os_log_return_address()
  message.withUTF8Buffer { (buf: UnsafeBufferPointer<UInt8>) in
    buf.baseAddress?.withMemoryRebound(to: CChar.self, capacity: buf.count) { str in
      withVaList(args) { valist in
        _swift_os_log(dso, ra, self.log_from_label(), self.type_from_level(level), message, valist)
      }
    }
  }
}

Unfortunately, the current log signature misses #dsoHandle and args, using #file, #function and #line instead. What do you think about adding two more arguments to the log function? Or maybe you have some alternative suggestion?

Thanks in advance!

@shpakovski shpakovski changed the title Introduce #dsoHandle and args in addition #file, #function and #line Introduce #dsoHandle and args in addition to #file, #function and #line May 6, 2019
@weissi
Copy link
Member

weissi commented May 6, 2019

@shpakovski Thanks for the issue. The reason I didn't add #dsoHandle is that with SwiftPM it almost always be the same for everything because SwiftPM by default does 'automatic linkage' which means it links it all together in one binary. Ie. it doesn't produce any .dylib/.so or .a files which is a good thing. Now I do realise that especially non-server people use .frameworks and then the #dsoHandle isn't always the same.

So if you use SwiftPM, you can just pass #dsoHandle of your current file to _swift_os_log :).

As you point out, without breaking public API we also can't add the dso to the protocol now. What we could in theory do is to add new methods to the protocol that do require the dso parameters but ship default implementations that delegate to the ones without the dso parameter.

I'm not sure however we should do this because as you point out you use a lot of private API and what you're doing isn't really guaranteed to work... I think we should wait until os_log releases their new API and then I would hope we have a better way of interfacing with it.

@shpakovski
Copy link
Author

shpakovski commented May 6, 2019

I need this for a Mac app which is full of frameworks 🙈 And #dsoHandle “brings” a location in source code, but I don’t have enough competence to discuss on this, sorry.

Not sure about this pitch anyway. Because of os_log args and public / private modifiers, swift-log cannot be reused here, probably. So I was just curious about the potential future compatibility.

@weissi It’s great that you replied with more details. And thanks a lot for consideration!

@DeFrenZ
Copy link

DeFrenZ commented Oct 18, 2019

I agree that interfacing with os_log through private APIs should be discouraged, and as such not supported by swift-log out of the box, but it should be possible to interface with it by writing the handler ourselves.
There are a few limits in the current interface that makes that impossible:

  • there is no way to pass the #dsoHandle, as mentioned upthread
  • the only data passed is Message (which is basically a String), and optional Metadata (keyed strings, with possible nesting). This leads to the impossibility of knowing at runtime wether — and if so which parts — of the message were static and which were dynamic (e.g. StaticString). Without this knowledge, functionality like the automatic redaction of dynamic elements cannot be used, and even if reimplemented you'd need a different Logger to be able to pass it through.

I did in a private project re-use a modified version of swift-log with workarounds such as passing a format and arguments instead of a Message, and having multiple versions of log as requirements instead to "solve" the variadic propagation issue, but I'd love to be able to use swift-log as-is without losing functionality like you would with current solutions.

Though as I understand there's no desire to break the API for this added functionality, with the chance that once a "Swifty" version of os_log comes forward might be incompatible 😞

Edit

To be more specific — since I nerdsniped myself it seems — if Message gets updated to something like this, then (with private APIs) os_log can be correctly (and nicely) used through this handler.

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

No branches or pull requests

3 participants