tonystone / tracelog

TraceLog is a highly configurable, flexible, portable, and simple to use debug logging system for Swift and Objective-C applications running on Linux, macOS, iOS, watchOS, and tvOS.
https://tonystone.io/tracelog
Apache License 2.0
53 stars 15 forks source link

Question about system logs #44

Closed felix91gr closed 6 years ago

felix91gr commented 6 years ago

Hi Tony and others (if there are) ^^

I'm helping at the langserver-swift project and we'd like to have some cross-platform logging. And since I know for a fact that TraceLog does work on both Darwin and Linux, I'm thinking it might be the perfect tool for the job.

Yesterday when we were talking about this, Ryan said that we should be outputting the log messages to Unified logging on Darwin and to the systemd logging journal on Linux. I think that's reasonable. Is is possible to configure TraceLog to do that? I know close to nothing about system logs so please forgive me if I'm noobing too much here. I know I probably am :sweat_smile:

tonystone commented 6 years ago

Hi @felix91gr, langserver-swift looks like a very cool project.

So on logging and TraceLog, yes, TraceLog was designed for use-cases like this, it allows multiple pluggable Writers that write the low-level information to various logging systems, databases, sockets, or whatever you'd like. You can install multiple so it can write the say the console and a file log at the same time. If you install your own writer, this removes the default writer.

The writer interface is straightforward, you basically just have to write the init code (if there is any ) and implement this method.

func log(_ timestamp: Double, level: LogLevel, tag: String, message: String, runtimeContext: RuntimeContext, staticContext: StaticContext)

For os_log you would use the os_log(_:dso:log:type:_:) method directly. The minor complication I see with this (and this would be with all loggers, not just TraceLog) is the #dsohandle, this is an opaque pointer that Apple uses to get the file, line, and func information for the calling context (TraceLog currently uses #file, #func, #line for capturing that). This would need to be passed in from the origin. TraceLog could easily be modified in a non-breaking fashion to capture this. I could do that for you.

The Writer for os_log would be straightforward, really, aside from the method and class declaration itself, just a one-liner. It also would be a worthy addition to TraceLog.

On the systemd journal. This should also be easy also I think there would be a little more to the writer since you would have to set up the address and ports to log to in the init method of your Writer, in the end, the write method would be very simple again. I believe in the shell you would use systemd-cat to log messages, not sure if that or another method is exposed in swift yet, would have to look into this. If it's not it would be a simple matter of creating a modulemap file to expose the C interface to Swift.

Both seem like worthy additions to TraceLog.


Update: I took a few moments to write an os_log writer (OSLogWriter) for TraceLog.

Take a look at branch os-log-writer.

Still need to figure out how to write tests for it and also may need a little cleanup.

felix91gr commented 6 years ago

This would need to be passed in from the origin. TraceLog could easily be modified in a non-breaking fashion to capture this. I could do that for you.

The Writer for os_log would be straightforward, really, aside from the method and class declaration itself, just a one-liner. It also would be a worthy addition to TraceLog.

Oh my gosh, that would be awesome! Please do :pray:

On the systemd journal. This should also be easy also I think there would be a little more to the writer since you would have to set up the address and ports to log to in the init method of your Writer, in the end, the write method would be very simple again. I believe in the shell you would use systemd-cat to log messages, not sure if that or another method is exposed in swift yet, would have to look into this. If it's not it would be a simple matter of creating a modulemap file to expose the C interface to Swift.

Both seem like worthy additions to TraceLog.

I'll look it up too. Let's see what we can find. If you need one, I could lend you a hand for testing this, since I have my Swift sat up on Linux :)

Again. thank you so much for your help :bowing_man:

tonystone commented 6 years ago

@felix91gr, would you want to contribute to the systemd Writer? We could work on it together. You have your environment setup, and you are familiar with your particular requirements, so that is a huge plus.

felix91gr commented 6 years ago

Sure :+1: I'd love to

felix91gr commented 6 years ago

Useful info for this:

Here's the documentation for logging into systemd.

I'm almost certain that we can't import it directly from Swift like we can import GLibc, but I need to read more on it to be sure. If not, we should make the modulemap.

Edit:

In the future, we might not even need modulemaps, so that'll be a cool day ^^. I don't think that's included in Swift 4.1, but it should be coming up for 4.2 or 5.0 :)


PS: I've had success in writing a logger that outputs logs into a file. Now that I understand that, I see why you chose such a design. Adding the canonical system logs would only require adding like 50 lines or less of Swift code in a new file :)

tonystone commented 6 years ago

That does indeed look like the correct calls. In addition, I found this helpful guide "systemd for Developers III", by Pid Eins.

Note that syslog can also be used (syslog is included in GLibc on Linux, see the modulemap for Glibc). The biggest issue I see with this is we would be missing the code file, func, line information which I believe is a show stopper for syslog.

From the sd_journal.h file I believe that the prefered calls for us would be:

   int sd_journal_print_with_location(int priority, const char *file, const char *line, const char *func, const char *format, ...)

Since this call contains the priority, file, line, and func which we would like to maintain.


P.S. TraceLog can certainly use a file writer so if you want to make your file work generic and contribute that, it would be a great addition. I could see it requiring the ability to choose a file location and name as well as file rotation and possibly some kind of max space it can take up before it starts to delete files.

RLovelett commented 6 years ago

I think that syslog is a variadic function. And last time I checked variadic functions are not supported by Swift without some sort of wrapper.

felix91gr commented 6 years ago

And last time I checked variadic functions are not supported by Swift without some sort of wrapper.

Ah, dammit. That may be why my attempts at making a Swift wrapper aren't working.

We'll need a C wrapper, then. That's not so hard to provide :) Gimme 10 minutes, I think I know what my code is missing now.

Edit: this is what I'm working on, btw

felix91gr commented 6 years ago

Okay, it's ready. But it still fails. I think it may be because I need to specify a pkgConfig command such that SPM can find the C header in my system.

But for using pkgConfig, the wrapper has to be a System Module package... Anyone here knows how to make one of those? I've tried the official documentation but the exact code posted there doesn't work :thinking:


Btw, for accessing systemd on Debian-based Linuxes, this might be necessary:

sudo apt-get install libsystemd-dev

felix91gr commented 6 years ago

I now just realized that you were talking about syslog. We're trying to work with sd_journal_print, which seems to be the modern replacement for syslog. But nevertheless the problem you pointed at (variadic functions not being accessible) is there. The sd_journal_print function is variadic as well.

felix91gr commented 6 years ago

And, last answer because I feel like I'm spamming y'all:

@tonystone,

From the sd_journal.h file I believe that the prefered calls for us would be:

int sd_journal_print_with_location(int priority, const char *file, const char *line, const char *func, const char *format, ...)

Since this call contains the priority, file, line, and func which we would like to maintain.

I'd tend to agree, but I don't think that's the intended usage of that function. If you take a look at the headers, it says: "Used by the macros below. You probably don't want to call this directly.". Can we make-do with sd_journal_print? Since it's part of the official API, we would be better off if we could use it instead.

P.S. TraceLog can certainly use a file writer so if you want to make your file work generic and contribute that, it would be a great addition.

Thanks for the offer! I'd like to do it, but maybe not right now. I have winter holidays in July, and with the break from uni I should be able to dedicate myself to that feature mindfully :)

Otherwise, please feel free to take the current implementation at traceLog-usage and make it complete with things like what you mentioned:

I could see it requiring the ability to choose a file location and name as well as file rotation and possibly some kind of max space it can take up before it starts to delete files.

To which btw, I agree :)

RLovelett commented 6 years ago

In my opinion, sd_journal_print is insufficient.

For example, let's say you make an executable called example. The only thing it does is log "Hello World".

import Clibsystemd

sd_journal_print_ryan(LOG_NOTICE, "Hello World");

Assuming sd_journal_print_ryan is just a wrapped sd_journal_print like so:

#include <systemd/sd-journal.h>

extern inline
int sd_journal_print_ryan(int priority, const char *format) {
    return sd_journal_print(priority, format);
}

In the log you'll see:

{
  "__CURSOR": "s=8f3c919c4ad74561ae7ff7f600b0c2c7;i=12b6a;b=d3f758437aad4cc29fb3a4bd9d25e7e2;m=55739513f8;t=56d2259da3a90;x=23d968a990554398",
  "__REALTIME_TIMESTAMP": "1527369187342992",
  "__MONOTONIC_TIMESTAMP": "367011369976",
  "_BOOT_ID": "d3f758437aad4cc29fb3a4bd9d25e7e2",
  "_TRANSPORT": "journal",
  "_UID": "1000",
  "_GID": "1000",
  "_CAP_EFFECTIVE": "0",
  "_SELINUX_CONTEXT": "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023",
  "_AUDIT_SESSION": "3",
  "_AUDIT_LOGINUID": "1000",
  "_SYSTEMD_OWNER_UID": "1000",
  "_SYSTEMD_UNIT": "user@1000.service",
  "_SYSTEMD_SLICE": "user-1000.slice",
  "_SYSTEMD_USER_SLICE": "-.slice",
  "_MACHINE_ID": "f039efa85fea4717ab5f8d0fbd83198e",
  "_HOSTNAME": "plex.local",
  "_SYSTEMD_CGROUP": "/user.slice/user-1000.slice/user@1000.service/dbus.service",
  "_SYSTEMD_USER_UNIT": "dbus.service",
  "PRIORITY": "5",
  "_SYSTEMD_INVOCATION_ID": "f306c6217fa144199a2904e1cd1fe86f",
  "MESSAGE": "Hello World",
  "CODE_FILE": "/home/rlovelett/Source/example/Packages/Clibsystemd/sd-journal.h",
  "CODE_LINE": "5",
  "CODE_FUNC": "sd_journal_print_ryan",
  "SYSLOG_IDENTIFIER": "example",
  "_COMM": "example",
  "_PID": "29947",
  "_EXE": "/home/rlovelett/Source/example/.build/x86_64-unknown-linux/debug/example",
  "_CMDLINE": ".build/debug/example",
  "_SOURCE_REALTIME_TIMESTAMP": "1527369187342964"
}

Pay attention to CODE_FILE, CODE_LINE and CODE_FUNC none of those are what I would want to see in a Swift program. Perhaps that is just me though.

felix91gr commented 6 years ago

You're right. We should be using sd_journal_print_with_location or sd_journal_printv_with_location then.

Also, I just realized that sd_journal_printv and sd_journal_printv_with_location are non-variadic. We could call sd_journal_printv directly from Swift and maybe get the CODE_FILE and such parameters correctly captured, or we could use a wrapper around sd_journal_print_with_location that asks por those parameters, like this:

#include <systemd/sd-journal.h>

extern inline
int csd_journal_printv_for_swift(int priority, const char *file, const char *line, const char *func, const char *format) {
    return sd_journal_printv_with_location(priority, file, line, func, format);
}

And then in Swift we'd then use the compile-time macros, but somehow we'd make them point to the caller's lines and files*:

// Here, I log...

sd_journal_printv_with_location(convertToSystemD(priority), #file, #line, #function, message)

* Possibly we'd make that happen with cross-module inlining? I have no idea.

tonystone commented 6 years ago

@felix91gr, TraceLog captures #file, #line and #function at the calling object, they simply need to be passed on to sd_journal_printv_with_location. See the StaticContext object. Also look at any one of the method declarations for logging. Tracelog captures the static context of the caller. Btw, the dynamic context is also captured which is the thread and other information about the calling line of code. The basic architecture of TraceLog is TraceLog core captures everything that is needed for logging and trace functionality, the Writers format and write that out to any service that needs them.

felix91gr commented 6 years ago

Ohhh, I see now. That is awesome :D

Then we just need this System Module to work... I'll investigate a bit more and ask at the forums if I don't manage to make it work.

felix91gr commented 6 years ago

I'm gonna try and base it off of what IBM has done here: CCurl.

They:

  1. Made a system module package
  2. Added code (directly in the .h file of the package) to have the API they wanted

(1) is of course what we're trying to have, and (2) allows us to solve the variadic arguments problem.

felix91gr commented 6 years ago

Okay, I'm done for today. I made it work almost all the way, I think.

Here's the project.

If you want to test it, use the tag 0.6.1

As I put in the release notes, what's missing is basically entering the logs to the system journal.

It compiles, and the same function, when used in a C program, logged my input into the journal with no problem.

So if you guys want to help find what's missing... I'd really appreciate it :) For now tho, I'm going to sleep ^^

tonystone commented 6 years ago

@felix91gr, I was trying this out and received the following, any thoughts?

Compile Swift Module 'TraceLogJournalWriter' (1 sources)
/vagrant/Sources/TraceLogJournalWriter/main.swift:26:1: error: use of unresolved identifier 'csd_journal_print'
csd_journal_print(0, #file, &line, #function, "Test message.")
^~~~~~~~~~~~~~~~~
Csdjournal.sd_journal_print:2:13: note: did you mean 'sd_journal_print'?
public func sd_journal_print(_ priority: Int32, _ format: UnsafePointer<Int8>!, _ varargs: Any...) -> Int32
            ^
Csdjournal.sd_journal_printv:1:13: note: did you mean 'sd_journal_printv'?
public func sd_journal_printv(_ priority: Int32, _ format: UnsafePointer<Int8>!, _ ap: CVaListPointer) -> Int32
            ^
error: terminated(1): /home/vagrant/swift-4.1-RELEASE-ubuntu16.04/usr/bin/swift-build-tool -f /vagrant/.build/debug.yaml main output:
felix91gr commented 6 years ago

What tag are you using? It must be one of the (many, oh so many) that didn't link properly.

Gimme a couple minutes, I've made a new tag and I'm making a dummy swift project that imports it, to demonstrate the current missing functionality.

tonystone commented 6 years ago

0.6.1

felix91gr commented 6 years ago

Try the 0.9.0, that's the last one I've put on GH. That should work

tonystone commented 6 years ago

Same issue on that. Hmm, let me upload this test project. You can take a look. btw: I'm running Ubuntu 16.04 (only version that supports libsystemd-dev) and Swift 4.1.

tonystone commented 6 years ago

https://github.com/tonystone/tracelog-journal-writer

felix91gr commented 6 years ago

Cool!

Okay, I've finished the setup. This is the project that shows how the wrapper fails.

felix91gr commented 6 years ago

In your project, I think this might be an unrelated problem?

You have csd_journal_print(0, #file, &line, #function, "Test message.").

And &line is a pointer to an int. That's technically a valid input to the function (since int pointers and char pointers are the same for C), but semantically you should be sending a string :)

tonystone commented 6 years ago

Yes, I knew about that, was simply focusing on getting it to compile first.

felix91gr commented 6 years ago

I'm running Ubuntu 16.04 (only version that supports libsystemd-dev) and Swift 4.1.

Me too, so that's awesome. We must have almost identical setups (modulo installed apt-get packages)

Yes, I knew about that, was simply focusing on getting it to compile first.

:thinking:

Maybe there's a package that your setup is missing, and that I didn't note down as a provider on the Package Manifest. Let's see: can you run the dummy project? It should give you an output like this:

felix@felix-X550LD ~/D/P/t/csd_proof> swift run csd_proof
Compile Swift Module 'csd_proof' (1 sources)
Linking ./.build/x86_64-unknown-linux/debug/csd_proof
I'm sending This is what I want to log to the system log, from myFile, at myFunc : myLine
Log has been sent!
I've recieved -22 out of the system call.
tonystone commented 6 years ago

Yes, the dummy project works, save the -22 return code.

tonystone commented 6 years ago

Ok, I figured it out. The Package.resolved was still pointing to a fork I made of your repo. Just had to delete it.

Looks good so far 👍

tonystone commented 6 years ago

@felix91gr, not sure if this would be useful to you but this is a script I wrote to startup a dev environment to any of the supported Linux and swift versions. I used it to quickly build environments. It's not perfect but it encapsulates a lot of work.

vagrant-siwft

For instance, this is the command I used to create a vm for your tests.

   vagrant --platform-version=16.04 --swift-release=4.1 up

You simply create a directory to work in and create a soft link to the Vagrant file.

  # mkdir Csdjournal-proof-of-bug
  # cd Csdjournal-proof-of-bug
  # ln -s ../vagrant-swift/Vagrantfile Vagrantfile
tonystone commented 6 years ago

@felix91gr, we'll need to create a new repo for the sd-journal writer. With the dependencies on the system library, I don't believe there is a way to isolate that from other platforms if it's in the main repo. If you know of a way, I'm open to that as well.

You can create a new repo in your account or you can have it live with mine, it's totally up to you of course. I think a good name to keep with the naming conventions I had in mind would be tracelog-sdjournal-writer or tracelog-journal-writer.

We can modify TreaceLog's readme to reference the new repo where ever it lives.

felix91gr commented 6 years ago

@felix91gr, not sure if this would be useful to you but this is a script I wrote to startup a dev environment to any of the supported Linux and swift versions. I used it to quickly build environments. It's not perfect but it encapsulates a lot of work.

It should be, for when I want to test how to set up something from scratch. I work using Linux; it's my main OS. But since I have almost everything I could need already installed, it's hard to test a tutorial in my pc as-is. I'll take a look, thank you! :)

we'll need to create a new repo for the sd-journal writer. With the dependencies on the system library, I don't believe there is a way to isolate that from other platforms if it's in the main repo.

That is correct. Afaik, there is no way to have nested packages yet, and the only (supported) way to access system modules is the way I'm using, which requires a dedicated Swift package for the wrapped library.

Regarding naming, the most common way to name C wrappers for Swift is to use the original name and precede it with the letter C:

Now. As it stands right now, the wrapper opens the <systemd/sd-journal.h> function namespace to Swift. It also includes a little shim that helps with usage of the variadic print_with_location function. The name should probably be something like CSDJournal or Csdjournal.

That said, if we're only gonna use it (at least for the moment) to help TraceLog write to the sdjournal, then it's not a project that we would actively support. Therefore, the standard naming doesn't make all that sense.

tl;dr I think the name should indeed be something like what you said. tracelog-sdjournal-writer or tracelog-sdjournal-shim sounds about right.

felix91gr commented 6 years ago

Okay, wait. I'm having a breakthrough. I think I know what's wrong with the wrapper. Brb

felix91gr commented 6 years ago

IT WORKS.

I'LL POST IT IN A BIT :D STAY TUNED

tonystone commented 6 years ago

Regarding naming, the most common way to name C wrappers for Swift is to use the original name and precede it with the letter C:

CLibAVUtil CCurl CEpoll

Now. As it stands right now, the wrapper opens the <systemd/sd-journal.h> function namespace to Swift. It also includes a little shim that helps with usage of the variadic print_with_location function. The name should probably be something like CSDJournal or Csdjournal.

That said, if we're only gonna use it (at least for the moment) to help TraceLog write to the sdjournal, then it's not a project that we would actively support. Therefore, the standard naming doesn't make all that sense.

Ah no, I wasn't saying rename the wrapper around systemd, my thought was the TraceLog.Writer implementation. I didn't think we could make TraceLog itself depend on Csdjournal as that would not compile on iOS, macOS, tvOS, and watchOS but I was just doing some testing and it seems as long as we surround the import CSDJournal and all code with #if os(Linux) it does compile. Even though the Package lists a depedency that can only be compiled on Linux.

I'll need to do some more confirmation on this though.

felix91gr commented 6 years ago

@tonystone @RLovelett It's complete: https://github.com/felix91gr/Csdjournal/releases/tag/1.0.0 :sparkles:

Check out the shim.h: the problem was in that we had to prepend the strings with things like CODE_FILE.

(That was not mentioned in the API, and that makes sense: it's a function that was intended for internal usage. I checked out how some of the macros worked, and indeed we were missing these strings. By adding them, everything worked out as if it always did :heart: )

felix91gr commented 6 years ago

Ah no, I wasn't saying rename the wrapper around systemd, my thought was the TraceLog.Writer implementation. I didn't think we could make TraceLog itself depend on Csdjournal as that would not compile on iOS, macOS, tvOS, and watchOS but I was just doing some testing and it seems as long as we surround the import CSDJournal and all code with #if os(Linux) it does compile. Even though the Package lists a depedency that can only be compiled on Linux.

I'll need to do some more confirmation on this though.

Yup, with #ifs you can pretty much have OS-exclusive dependencies. That's one of the perks of the Package Manifest being written in Swift itself.

Take a look at this for inspiration: SKD dependencies

tonystone commented 6 years ago

It's complete: https://github.com/felix91gr/Csdjournal/releases/tag/1.0.0

Check out the shim.h: the problem was in that we had to prepend the strings with things like CODE_FILE.

Glad you figured out the prepending of the strings. I do see an issue we are going to have with the implementation as is though. The buffers used are fixed length, file and function from Swift are going to be larger than the 60 allocated on the stack. The file comes back as the entire path on Linux.

The function can also be quiet large.

felix91gr commented 6 years ago

Ah, well, that shouldn't be a problem. Since those are stack-allocated, we should be fine if we use huge ones (like size 200-400).

You tell me the size and I'll put it :)


We could also use dynamically-allocated buffers, now that I think of it. But I'd like to know if we can have those and:

a) Not suffer from buffer-overflow attacks b) Not have a noticeable impact on performance (from malloc and such)

What do you think about it? I can make it dynamic, if it's safe and fast :)

On Sun, May 27, 2018 at 9:46 PM Tony Stone notifications@github.com wrote:

It's complete: https://github.com/felix91gr/Csdjournal/releases/tag/1.0.0

Check out the shim.h: the problem was in that we had to prepend the strings with things like CODE_FILE.

Glad you figured out the prepending of the strings. I do see an issue we are going to have with the implementation as is though. The buffers used are fixed length, file and function from Swift are going to be larger than the 60 allocated on the stack. The file comes back as the entire path on Linux.

The function can also be quiet large.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tonystone/tracelog/issues/44#issuecomment-392398018, or mute the thread https://github.com/notifications/unsubscribe-auth/ALNBJ0eBZPFlZ6sjzky3H1rTqiyTHFwbks5t21cPgaJpZM4UNXFm .

felix91gr commented 6 years ago

Okay, I think I've convinced myself that we should indeed use dynamic allocation.

  1. Buffer-overflow attacks shouldn't be possible if we're calling this from Swift, as Strings are nil-terminated by construction. A user of the wrapper would expose themselves only if they were doing something like manipulating the String buffers of the message to be logged, directly. And I don't see why we'd ever want to do that.
  2. C is faster than Swift. A couple of malloc and frees under clang's optimizer shouldn't make a noticeable hitch in performance, specially if on the Swift side we're working with much more expensive operations.

So, release 1.0.1 now has dynamic allocation:

Thanks for the feedback, @tonystone ^^

felix91gr commented 6 years ago

Last update: I've updated the dummy project so that it uses the latest version of the wrapper. You can see how to use the 8 levels of logging in there.

One of the nicest things about how Swift imports C is that you can use the int constants directly. No need to keep a copy of the value table in code; you can just write LOG_NOTICE and you'll get the right int constant for that priority.

And, now I'm going to bed. Have a great week, you two :slightly_smiling_face:

felix91gr commented 6 years ago

Second update: @RLovelett found a bug that made it hard to use the library in more than one source file at a time. That's now been fixed on version 1.2.0:

tonystone commented 6 years ago

Nice work @felix91gr @RLovelett 👍

felix91gr commented 6 years ago

No problem!

Guys, I'll be stepping out for the week. I've got an assignment to finish for Friday and I'm quite behind 😅

Please feel free to fork the wrapper if you need to make changes. I hope to be back around Saturday-Sunday. Til' then! :)

tonystone commented 6 years ago

@felix91gr @RLovelett, just want to throw this out there since I spent a little time checking out an alternative to the shim wrapper. My thought was that sd_journal_sendv was the lowest level method that all the other methods eventually call (and I validated that with the source). This call allows Swift to call it directly and also provides for all the various fields we would need plus any custom ones such as TraceLogs tags, etc.

The calling code is a bit more complicated but it does move all the code into Swift (literally there is no C code, just a reference to the header file.

I see advantages to both the csd_journal_print and the sd_journal_sendv direct call.

Please see:

Note that the header in the system module now only contains:

#import <systemd/sd-journal.h>

There is an explanation of why that is in comments.

Calling code is more complicated but much more flexible and allows any of the params that sd-journal allows plus custom fields.

var utf8Text  = ["MESSAGE=Test Message".utf8CString,
                 "CODE_FILE=\(#file)".utf8CString,
                 "CODE_LINE=\(#line)".utf8CString,
                 "CODE_FUNC=\(#function)".utf8CString,
                 "PRIORITY=\(LOG_INFO)".utf8CString
]

var array: [iovec] = []

for i in 0..<utf8Text.count {
    let count = utf8Text[i].count - 1
    utf8Text[i].withUnsafeMutableBytes { (bytes) -> Void in
        if let address = bytes.baseAddress {
            array.append(iovec(iov_base: address, iov_len: count))
        }
    }
}

sd_journal_sendv(&array, Int32(array.count))

Happy to use either, just wanted to offer you guys this so I didn't feel like I waisted the research :-)


Btw: you can find the source for all the calls in discussion here journal-send.c

felix91gr commented 6 years ago

Okay, I managed to solve one third of this assignment quickly (phew). I'll give my two cents right away, because I don't want you guys to wait on me or this :)

@tonystone, I think this is key:

allows any of the params that sd-journal allows plus custom fields.

What extra parameters does it allow? How could we make use of that? That sounds interesting.


Comparing the two solutions, this is what I think:

The shim.h is clearer for me to read. That might be because the whole pointer arithmetic and memory manipulation thing is first-class there. Therefore, it feels like an easier code to debug and maintain. And given that we're working with C, that's pretty nice to have.

I still think both solutions are pretty close. If the extra flexibility of the Swift-side wrapping seems worth it, that would tip the scales.

Take me with a grain of salt here as well, because (1) I made the shim.h and I can't 100% take away my bias ^^. Also (2) I find that level of C code fairly easy to read because I had to work with C a lot in the last couple of years. That might not be all that common.

What do you think?

tonystone commented 6 years ago

@felix91gr, so my philosophy of combining languages for the last 30+ years has been to always stick to the primary language the system, tool, app, etc is being written in and only go to a lower level language for speed or if it just can't be done in the primary language. This keeps the code more maintainable by others who may not be as adept at mixing languages as us.

I believe your C implementation is very nicely written, understandable and maintainable. It is hard to beat a single language module though.

So you got me thinking again when I was answering this question about simple ways to do this and I came up with a simpler no C option directly calling sd_journal_printv_with_location since it's a va_list function and not variadic. Here's the call to that which is very simple indeed.

withVaList([]) { vaList -> Void in
    sd_journal_printv_with_location(LOG_INFO, "CODE_FILE=\(#file)", "CODE_LINE=\(#line)", #function, "Test Message", vaList)
}

Note: In the above, we do lose the ability to send extra params, but the only one I can think of after reviewing the list of params that are already present(see list here) is the declared Tag in TraceLog.

So in the end, even though I love C and your implementation, the simplicity of not having C and the above call drives me toward the no C direct option.


Btw: I finally figured out why function did not require a prefix and investigating it I found this.

        /* func is initialized from __func__ which is not a macro, but
         * a static const char[], hence cannot easily be prefixed with
         * CODE_FUNC=, hence let's do it manually here. */
       ALLOCA_CODE_FUNC(f, func);

All the higher level functions handle prefixing that for you, the only one that does not of course is sd_journal_sendv which is the lowest call in the stack.

felix91gr commented 6 years ago

I believe your C implementation is very nicely written, understandable and maintainable.

Thank you <3

My philosophy of combining languages for the last 30+ years has been to always stick to the primary language the system, tool, app, etc is being written in and only go to a lower level language for speed or if it just can't be done in the primary language. This keeps the code more maintainable by others who may not be as adept at mixing languages as us.

Okay you raise a fair point here. I didn't think of this, but it makes much more sense for this to be written as much in Swift as it is feasible (i.e. possible and performant). I believe there's nothing much C can do against the performance of the Swift-C interop APIs, at least not in a noticeable way by our users. Therefore, just for maintainability and ergonomics sake, we should keep this logging interface at the Swift layer whenever possible. I agree with you.

(...) and I came up with a simpler no C option directly calling sd_journal_printv_with_location since it's a va_list function and not variadic. Here's the call to that which is very simple indeed.

It is! The other day I took a look at those functions, but gave up as I don't quite understand how to use va_lists. Can we create one of those from Swift? If so, that would be fantastic! So much easier to use :)

Btw: I finally figured out why function did not require a prefix and investigating it I found this.

Ohh, that's very interesting indeed.

tonystone commented 6 years ago

It is! The other day I took a look at those functions, but gave up as I don't quite understand how to use va_lists. Can we create one of those from Swift? If so, that would be fantastic! So much easier to use :)

See the previous comment, the first piece of code is Swift creating a va_list although in this case its an empty list. Could just as easily of had a few params. Here it is again:

///
/// Swift code creating a va_list or technically a `CVaListPointer` which is a wrapper 
/// around a C `va_list`.
///
/// Note the empty array in `withVaList`, that is where any args would go that are in the va_list.
///
withVaList([]) { vaList -> Void in
    sd_journal_printv_with_location(LOG_INFO, "CODE_FILE=\(#file)", "CODE_LINE=\(#line)", #function, "Test Message", vaList)
}

I updated my example so you can try it out.

felix91gr commented 6 years ago

I actually works. Holy shit, this was so easy!

What are va_list from Swift? Are they arrays of strings? Arrays of.... Anything?