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: Enable ETW logging #3036

Merged
merged 3 commits into from
Feb 22, 2019
Merged

Conversation

lowenna
Copy link

@lowenna lowenna commented Feb 21, 2019

Signed-off-by: John Howard jhoward@microsoft.com

Enables ETW logging on Windows. (https://docs.microsoft.com/en-us/windows/desktop/etw/event-tracing-portal as a starting point for more information)

@kevpar @jterry75 FYI.

@jterry75
Copy link
Contributor

Nice! LGTM (fix the build break with the extra return)

@jterry75
Copy link
Contributor

@jhowardmsft - Do we want to remove the event log logging hook?

@kevpar
Copy link
Member

kevpar commented Feb 21, 2019

@jterry75 don't think we should remove the event log hook yet. We can register a service to cause these ETW events to show up in event log, and then removing it's a good idea, but we should do that first in case anyone relies on it.

@jterry75
Copy link
Contributor

@kevpar - I agree for Docker but this is new in CD anyways. Should we just effectively 'never add it'?

@kevpar
Copy link
Member

kevpar commented Feb 21, 2019

@jterry75 We might want to check with Nick first, I think event log -> fluentd might be their plan for logs.

Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna
Copy link
Author

lowenna commented Feb 21, 2019

Nice! LGTM (fix the build break with the extra return)

Removed. (Odd that lint didn't flag it on my dev box, but whatevs....)

@lowenna
Copy link
Author

lowenna commented Feb 21, 2019

@kevpar - I agree for Docker but this is new in CD anyways. Should we just effectively 'never add it'?

@jterry75 Let's take any action on this as a follow-up. Not related to this PR itself.

@crosbymichael
Copy link
Member

Looks like the vendor on linux is broken with this change.

These files were modified:
 M vendor/github.com/Microsoft/go-winio/internal/etw/etw.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/eventdata.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/eventdatadescriptor.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/eventdescriptor.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/eventmetadata.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/eventopt.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/fieldopt.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/provider.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/providerglobal.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/ptr64_32.go
 M vendor/github.com/Microsoft/go-winio/internal/etw/ptr64_64.go
 M vendor/github.com/Microsoft/go-winio/pkg/etwlogrus/hook.go

The command "../project/script/validate/vendor" exited with 1.

@lowenna
Copy link
Author

lowenna commented Feb 21, 2019

@crosbymichael Fixed now. Some Windows-style line endings had crept into go-winio. Updated that repo, cut a new release and updated the vendoring commit again.

@codecov-io
Copy link

codecov-io commented Feb 21, 2019

Codecov Report

Merging #3036 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3036   +/-   ##
=======================================
  Coverage   43.87%   43.87%           
=======================================
  Files         102      102           
  Lines       10903    10903           
=======================================
  Hits         4784     4784           
  Misses       5384     5384           
  Partials      735      735
Flag Coverage Δ
#linux 47.48% <ø> (ø) ⬆️
#windows 41.08% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0769763...d83e4e9. Read the comment docs.

@crosbymichael
Copy link
Member

Overall, this looks good. I think we can move the initialization in an init() just to clean up the main.

John Howard added 2 commits February 21, 2019 14:16
Signed-off-by: John Howard <jhoward@microsoft.com>
Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna
Copy link
Author

lowenna commented Feb 21, 2019

Overall, this looks good. I think we can move the initialization in an init() just to clean up the main.

@crosbymichael Yes, makes sense. Updated.

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit dea27b1 into containerd:master Feb 22, 2019
@lowenna lowenna deleted the jjh/etw branch February 22, 2019 02:52
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

Successfully merging this pull request may close these issues.

5 participants