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

Add etw UnicodeStringField #211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pavelbazika
Copy link

Allow to send UTF-16 encoded message. It's the only missing part needed for writing messages with local characters into event log via this library, when string is specified as win:UnicodeString in instrumentation manifest (which cannot be changed).

UnicodeStringArray is there only for completeness from my point of view.

@pavelbazika pavelbazika requested a review from a team as a code owner May 5, 2021 18:38
@ghost
Copy link

ghost commented May 5, 2021

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@anmaxvl anmaxvl left a comment

Choose a reason for hiding this comment

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

LGTM. can you commit with -s?

Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

lgtm also, same ask as above though. Can do git commit --amend --sign-off and repush.

// UnicodeStringArray adds an array of UTF-16 strings to the event.
func UnicodeStringArray(name string, values []string) FieldOpt {
return func(em *eventMetadata, ed *eventData) {
em.writeArray(name, inTypeUnicodeString, outTypeString, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want the out type here? It looks like TraceLoggingWideString in TraceLoggingProvider.h just uses TlgInUNICODESTRING with no out type.

Copy link
Author

Choose a reason for hiding this comment

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

Please see below.

@pavelbazika pavelbazika force-pushed the etw-unicodestring branch 2 times, most recently from 724e7f4 to 48d3521 Compare June 29, 2021 08:19
@kevpar kevpar self-assigned this Sep 9, 2021
@helsaawy
Copy link
Contributor

@pavelbazika
If you are still interested in merging this, could you rebase and push? it should be pretty quick to merge after that

Signed-off-by: Pavel Bazika <kokes@kocovnici.cz>
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

Everything looks good except for the linter (and for some reason the signing check expects your email to be different):

Commit sha: fc43157, Author: Pavel Bazika, Committer: Pavel Bazika; Expected "Pavel Bazika pavel.bazika@icewarp.com", but got "Pavel Bazika kokes@kocovnici.cz".

Comment on lines +35 to +36
binary.Write(&ed.buffer, binary.LittleEndian, unicode)
ed.buffer.Write([]byte{0, 0})
Copy link
Contributor

Choose a reason for hiding this comment

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

to suppress the linter on error checking (or you can add a //nolint:errcheck directive)

Suggested change
binary.Write(&ed.buffer, binary.LittleEndian, unicode)
ed.buffer.Write([]byte{0, 0})
_ = binary.Write(&ed.buffer, binary.LittleEndian, unicode)
_, _ = ed.buffer.Write([]byte{0, 0})

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.

None yet

5 participants