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

Windows: Add ETW logging driver plug-in #19689

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Windows: Add ETW logging driver plug-in #19689

merged 1 commit into from
Feb 16, 2016

Conversation

cednation
Copy link
Contributor

Signed-off-by: Cedric Davies cedricda@microsoft.com
@jhowardmsft

Adds an ETW logging driver to Docker on Windows. ETW stands for Event Tracing in Windows, and is the common framework for tracing Windows applications. This log driver forwards each container log message as an ETW event. A client can then create an ETW listener to listen for these events.

A client registers to listen to events from our ETW provider, by specifying our provider's GUID, which is: a3693192-9ed6-46d2-a981-f8226c8363bd. Here is an example of how to listen to these events using the logman utility available in any Windows installation:

  1. logman start -ets DockerContainerLogs -p {a3693192-9ed6-46d2-a981-f8226c8363bd} 0 0 -o trace.etl
  2. Run your container(s) with the etwlogs driver, by adding --log-driver=etwlogs to the Docker run command, and generate log messages
  3. logman stop -ets DockerContainerLogs
  4. You can then convert the generated etl file to XML using: tracerpt -y trace.etl

Each ETW event generated will contain a data string in this format:
container_name: %s, image_name: %s, container_id: %s, image_id: %s, source: [stdout | stderr], log: %s

Your ETW listener can then parse this string, to both get the log message generated, as well as its context information, such as which container emitted the log message. The Timestamp is also available within the ETW event.

@MHBauer
Copy link
Contributor

MHBauer commented Jan 25, 2016

You might as well squash the three commits together. Having golint and gofmt in separate commits doesn't really add anything helpful to the history. Thanks for the contribution!

@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jan 26, 2016
@cednation
Copy link
Contributor Author

@MHBauer Squashed the commits into one. Thanks!

@calavera
Copy link
Contributor

🙌 LGTM 🙌

@MHBauer
Copy link
Contributor

MHBauer commented Jan 29, 2016

@cednation Looking good. Thanks for the contribution. What will really polish this off is to add an entry to docs/admin/logging/overview.md and perhaps a docs page of it's own.

I wish I knew more about windows programming, but I have heard the praises of ETW being sung. I can't give much more than general thoughts about the code itself. It looks complete for an implementation of a logger. I like that there are debug logs.

I feel like I have seen the fixContainerName function elsewhere, and wonder if there is a utility that can be provided, or a potential for a new function on the Container object (That change is out of scope here, but that's my thought).

return err
}
// The provider's GUID is {a3693192-9ed6-46d2-a981-f8226c8363bd}
guid := syscall.GUID{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is syscall.GUID only in the Windows version of golang? I had problems trying to find documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MHBauer Hi MGBauer, thanks for the review comments! Your suggestion to add documentation to the overview.md file is a great one. I've done that, and also added more detailed documentation in a new etwlogs.md file (trying to follow the format used by the other logging drivers). I've updated the pull request with these additional changes.

As per syscall.GUID, yes I believe this is only in the Windows version. I see the type is defined within syscall/ztypes_windows.go. It is common to use GUIDs within Windows programming.

@MHBauer
Copy link
Contributor

MHBauer commented Jan 29, 2016

@cednation good start on the docs. 👍 I've left a couple of comments. To check how the documentation renders, issue a make docs and then open a browser to the address it spits out at the end.

@cednation
Copy link
Contributor Author

@MHBauer Thanks for the helpful comments and your patience. I never modified a md file before and wasn't sure how to test the rendering. I've fixed and updated everything you recommended and tested the rendering. I fixed the table, and made other items verbatim strings or code snippets as appropriate. And replaced all my apostrophes with backticks. :) Please let me know if you see any other things in the docs that need updating.

Oh and on the utility method for fixContainerName, I'm happy to switch to a common utility method if there is one, but I couldn't find one. I agree there probably should be, but I don't know if it would be appropriate for me to add it.
I have noticed that at least one other logging driver strips out the leading '/' as I am doing, but some others, for example fluentd, do not. And of course the docker client strips it out when you do a dockder ps. Personally, I feel the leading '/' is not helpful for the user to see, but I wouldn't mind keeping it in the output if you feel that is more consistent, I would just update my listener's regex to strip it out.


The ETW provider that this logging driver registers with Windows, has the
GUID identifier of: `{a3693192-9ed6-46d2-a981-f8226c8363bd}`. A client creates an
ETW listener and registers to listen to events from the logging driver`s provider.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this backtick ` can be a regular single quote '

@MHBauer
Copy link
Contributor

MHBauer commented Jan 29, 2016

I think the docs are looking real good, let's get @thaJeztah & @moxiegirl to take a look.

I wouldn't worry about the container name fix now. I'll put it in my list of things to follow up on. This is a very nicely single self-contained piece of work. Let's keep it like that for now.

Thanks for being so understanding and accommodating of changes.

@thaJeztah
Copy link
Member

@MHBauer yup, docs already look good (at a glance); but let's get it moved to "docs review" first

@cednation looks like this needs a rebase 😢

@cednation
Copy link
Contributor Author

@thaJeztah @MHBauer I'm very confused on how to properly do a rebase. Can you offer any advice? I don't understand why when I updated the docs last time, did a commit --amend, and then 'git push -f' that that didn't work. (I know you would want me to amend the older commit, so that there is only one commit in the history)
I tried to fix things by doing a git fetch upstream, git pull, and then git rebase -i. But since there were about 200 entries here, I thought I could do the rebase faster non-interactively, and did 'git rebase'. Now when I push, I get 98 commits being pushed.
I'm sorry, Git is very new to me as you can probably tell.
Do you know what I should do now to fix things? Or should I just close this review and start all over from scratch?

@cednation
Copy link
Contributor Author

@thaJeztah @MHBauer Also, when I was trying to do the interactive rebase, it said I couldn't squash the first commit, which wasn't mine, but I thought I was supposed to squash every commit other than mine.... so wasn't sure how to proceed.

@MHBauer
Copy link
Contributor

MHBauer commented Jan 30, 2016

This looks like you rebased master on top of your branch instead of your branch onto master. Rebases are directional. I usually:

git checkout ${MY_BRANCH}
git fetch
git rebase origin/master

I'm going to need to think a minute about your case.

@thaJeztah
Copy link
Member

@cednation you can find some instructions in our contributors guide; https://docs.docker.com/opensource/workflow/work-issue/#pull-and-rebase-frequently

@cednation
Copy link
Contributor Author

@thaJeztah Those are the instructions that got me screwed up in the first place! :)
I followed those instructions, but when I got to "Replace the pick keyword with squash on all but the first commit", I got confused about following that, because the first commit wasn't my commit. Also there were over 100 commits to squash, so I thought there must be an easier way. (On Monday when I did the same thing with an older branch, there were over 600 commits, after doing a search & replace, it still didn't work right, and I was forced to start over.)
So then I tried just "git rebase" and that's what ended up getting me into the state that I am in. Not sure how to reverse course now... Maybe I need to delete everything and start over?

@MHBauer
Copy link
Contributor

MHBauer commented Jan 30, 2016

Alright, be very very careful. If it breaks, you get to keep both pieces!

git reset --hard 56e5a26251b0fe3ea51532a31527af22967c8fba   # This is your commit
git rebase upstream/master # I left this step out at first
git push -f

Make sure to save any work you have in progress in your local copy. Zip up the directory ahead of time if you want.

This assumes that you have your git remotes set up as in the docs, with upstream as the docker fork and origin as your github fork.

I did that and went from https://github.com/MHBauer/docker/commits/etw-docs to https://github.com/MHBauer/docker/commits/etw-docs-rebased

@MHBauer
Copy link
Contributor

MHBauer commented Jan 30, 2016

I forgot to rebase in that, sorry! You can contact me if a chat would help! Sorry!

@lowenna
Copy link
Member

lowenna commented Jan 30, 2016

@cednation I sent you some instructions which will hopefully help. Otherwise, catch me on Monday and I'll walk you through it :)

@cednation
Copy link
Contributor Author

@MHBauer @jhowardmsft Thanks for your help!! I used the combined information that you sent me to fix my git history and resubmit.

@cednation
Copy link
Contributor Author

@MHBauer @thaJeztah One question I have when you are reviewing the etwlogs.md file, do you think it is worthwhile for me to include the regular expression help in the document, even though maybe it isn't the best regex expression that could be used. For instance, I originally had a mistake in it, and that I didn't realize '.' and '-' were valid characters for container names, which I have since updated. Maybe I should just remove it altogether, or simplify it to capture everything up to the comma, in case the allowable characters in a container name ever changes.

@cednation
Copy link
Contributor Author

The windowsTP4 failure doesn't look like something I would have caused. Problem with the build machine?

ln: failed to create hard link '/d/CI/CI-94413db/binary/docker-94413db.exe': File exists
07:35:23 ERROR: Failed to link

@albers
Copy link
Member

albers commented Feb 15, 2016

@cednation I'll add the bash completion after this PR is merged in order to avoid unnecessary dependencies among PRs.

@thaJeztah
Copy link
Member


The ETW provider that this logging driver registers with Windows, has the
GUID identifier of: '{a3693192-9ed6-46d2-a981-f8226c8363bd}'. A client creates an
ETW listener and registers to listen to events from the logging driver`s provider.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the back-tick in driver's to a regular quote?

@thaJeztah
Copy link
Member

I didn't see anywhere to update in 3 of the files you mentioned in the command reference

That would be my mistake; I thought we also mentioned the available types there, just like in the man pages (perhaps we should)

@thaJeztah
Copy link
Member

Some nits, but docs overall looking good already

@cednation
Copy link
Contributor Author

@thaJeztah Thanks for all your helpful comments. I have made all of the changes that you suggested. Let me know if there is anything else that I should change.

@thaJeztah
Copy link
Member

docs LGTM (thanks @cednation)

ping @MHBauer @vdemeester PTAL

@@ -185,7 +185,7 @@ unix://[/path/to/socket] to use.
**--label**="[]"
Set key=value labels to the daemon (displayed in `docker info`)

**--log-driver**="*json-file*|*syslog*|*journald*|*gelf*|*fluentd*|*awslogs*|*none*"
**--log-driver**="*json-file*|*syslog*|*journald*|*gelf*|*fluentd*|*awslogs*|*etwlogs*|*none*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason splunk isn't in here and it is in the other two?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think someone forgot to add it here.

@MHBauer
Copy link
Contributor

MHBauer commented Feb 16, 2016

I don't know enough about enabling logging on the daemon to know whether my nit is even a nit or not. LGTM otherwise.

@cednation
Copy link
Contributor Author

@MHBauer AFAIK the lists should be the same in all 3 cases. For the daemon it just specifies the default logging driver, so splunk should be in the list. I will add this minor change.

Signed-off-by: Cedric Davies <cedricda@microsoft.com>
@MHBauer
Copy link
Contributor

MHBauer commented Feb 16, 2016

re-LGTM. fingers crossed.

calavera added a commit that referenced this pull request Feb 16, 2016
Windows: Add ETW logging driver plug-in
@calavera calavera merged commit c795d0b into moby:master Feb 16, 2016
@calavera calavera deleted the etwlogs branch February 16, 2016 23:01
@cednation
Copy link
Contributor Author

@calavera Thanks!

@thaJeztah
Copy link
Member

@cednation congrats on your first PR merged! 🎉

@MHBauer
Copy link
Contributor

MHBauer commented Feb 17, 2016

Welcome @cednation! Congratulations! 😈

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

Successfully merging this pull request may close these issues.

8 participants