search

microsoft / go-winio

Win32 IO-related utilities for Go
MIT License
952 stars 182 forks source link

WriteEventString writes a single ETW event from the provider. #262

Closed thaJeztah closed 1 year ago

thaJeztah commented 2 years ago

This function allows writing a single, non-structured (textual) ETW event.

In moby/moby, we were maintaining a local implementation for our etwlogs driver (originally implemented in https://github.com/moby/moby/pull/19689).

I'm in the process of replacing that implementation with the code from go-winio (https://github.com/moby/moby/pull/44289) so that we can have a single "canonical" implementation; most bits were already supported by go-winio, except for this one.

⚠️ I realise this is not the "right" approach; this PR is mostly a "straight" port from what we had. Like other functions in this package;

So, I thought; let me open this as a "proposal" PR to see if adding this function is acceptable, and hoping someone could help moving it over the finish line (write and test the generator comments).

thaJeztah commented 2 years ago

@kevpar @helsaawy ptal 🤗

I'm also wondering of (some of) these would make sense to be upstreamed to golang.org/x/sys/windows (if so, I'm happy to draft up a PR for that).

helsaawy commented 1 year ago

@thaJeztah I implemented what I think is the best of both worlds here based on the docs saying this function should not be used. (I dont know what the correct way to collaborate on a PR is, maybe I need to open a PR against your branch?)

It logs at level 0 without a task name (just like your code), but the events will have the message in a field called "Messge" instead of just containing the raw string. I do not think this will be a problem (though let me know if otherwise), and keeps with the code aligned with TraceLogging.

thaJeztah commented 1 year ago

based on the docs saying this function should not be used.

Interesting. I'll be honest; I don't know a lot about this area. Curious why that specific API was picked at the time.

In general, I think (in moby) we should probably look if we can change these logs to be more structured, but that might be a breaking change, so my first goal was to keep the existing approach, but unify implementations (where possible).

I dont know what the correct way to collaborate on a PR is, maybe I need to open a PR against your branch?

My PR was a very quick one, that (effectively) just copies the code we have in Moby.

Feel free to open a separate PR with your changes 👍

It logs at level 0 without a task name (just like your code), but the events will have the message in a field called "Messge" instead of just containing the raw string. I do not think this will be a problem (though let me know if otherwise), and keeps with the code aligned with TraceLogging.

Sounds good! (as mentioned) I'm not very familiar with this, so could use some quick 👀 from others to confirm that change would be ok; let's do so on your PR (once you have time to open one).

Thank you!

helsaawy commented 1 year ago

Created #263, if you want to close this in favor of that

thaJeztah commented 1 year ago

Closing in favour of https://github.com/microsoft/go-winio/pull/263 👍