Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ETW exporter how-to - usage instructions document #628
ETW exporter how-to - usage instructions document #628
Changes from all commits
bd28bb0
fbd8142
c7111e0
3616cfc
fb4f5d3
16e94a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I am missing something, but where is this helper class
Properties
defined in ETW namespace ? Had a quick look to the code, but didn't find it there ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I might have missed the
using
aliases. I'll refresh it. It's presently defined here, in a PR which contains the code. I wanted to scope this PR to documentation only. This is the code:opentelemetry-cpp/exporters/etw/test/etw_tracer_test.cc
Line 26 in 48991c0
Going forward I want to benchmark the pros and cons of passing a container vs. initializer list, the topic we discussed with Johannes above. In general, the concept is not necessarily unique to ETW. It could be applied to other exporters. Maybe I'd propose a refactor of pulling it in a separate (optional) helper class on API surface. Users may still use the existing initializer lists as well, as this does not break the "API". There is a moment where we have to be careful about "ABI". But since the implementation is header-only and linked straight into the app, with the matching runtime, the "ABI" compatibility issue is not applicable in this scenario. I can elaborate on this offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also want to make sure that even if we pass in a reference to map, if there is a need to involve a transform to API call across the ABI boundary, then the container itself is
KeyValueIterable
-interface compatible. Means the SDK code may iterate over that one in ABI safe manner, and perform a copy of all key-value pairs to the actual container beyond the ABI boundary. This will only be needed for other (non-ETW) exporters, and ETW exporter is lucky in this regard. As it's currently done fully as header-only, slim, very few hops to the actual "Event Write" routine, with no shared library. Maybe I can contribute an example that canfork
the incomingevent properties container
(Event) to both - ETW and some other destination, such as Standard Output exporter for illustrative purposes (in contrib repo). Basically, an example that shows how to forward that container, dual-home to twoTraceProvider
. An example of aMetaTraceProvider
, that aggregates within itself two different OpenTelemetry provider implementations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit misleading in the context of this example. As far as I can see, none of the usages of
Properties
here is more efficient than what the API provides out of the box. There's nomemcpy
involved in this case:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer - really depends. When we need to decorate the properties with additional attributes, such as - passing non-const properties, append some common properties... Maybe a set of common properties unique to custom
TracerProvider
. For example - some sort of custom correlation technique that needs to be appended on every record. Let's say, some custom field calledcV
and stamped on everyProperties
object passed in; OR appending a group of otherdevice.*
orapp.*
fields on top.. Then we could have reused the original container that was created in the customer code once, then passing it around as non-const by reference on that same thread. Thus, not requiring a transform toRecordable
, and not needing the key-value iteration on it. And not needing to construct it in the SDK from initializer list as an argument...I think I should have moved this comment from
Attributes
down toevent Properties
below, to make a stronger case 😄 Because withAttributes
, it appears that what you are suggesting is indeed looking nicer. Maybe I should capture both. I can remove the comment if you don't like it, but I see scenarios where it would be more efficient.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this code reference as an example where accepting the collection, then passing this original collection around by reference, decorating it with additional contents, stripping unneded fields, etc. would be more efficient than trying to construct a new object (or a copy of an object) from initializer list. I think I understand your concern that I should've elaborated on exactly what scenarios would (in future) may benefit from passing in the original collection. This should work great for a header-only library, as we later on may completely bypass the copy/iteration/creation of a new object from initializer list, operating on original customer-code created object.
Structure-wise, both initializations look similar: initializer list as an argument to template, or a reference to container that's been already populated in customer code.
We mostly follow this pattern in this "other" library elsewhere:
https://github.com/microsoft/cpp_client_telemetry/blob/master/lib/api/Logger.cpp
And the practice of passing around the container prepared in customer code, e.g. literally passing around a modifyable map of variants, - has been working well. Especially when the original collection passed to SDK needs additional "decoration".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is another scenario: let's say your
collection does not change
and contains something likeappName=x,status=Failed,scenario=FileOpen
. You'd prepare it once in a static initializer. Then you pass it as a reference toProperties
toAddEvent
, never destroying it, thus - never needing to construct/reconstruct it. Appears like in the other case (if you used the initializer list instead of passing collection by reference), you'd end up constructing it from Key-Value Iterable every time? It appears like construct-once-then-reuse will be more optimal then, at least for a statically linked code where there is no passing across ABI boundary needed, no key-value iterable transform. I can add some benchmarks to measure those scenarios in my other (code) PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the specific use-case/scenario as an example in this doc or in unit-test ( and refer it here ) to justify the performance statement. As the current api/sdk implementation, the only place where
memcpy
MAY happen is during Recordable::AddEvent() execution, but that would depend how exporter implements this logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that is the most commonly invoked function, actually. In ETW use case - it is entirely header-only, and it entirely avoids transform to
Recordable
upfront. The idea is thatProperties
may not even need to be transformed toRecordable
. Because it stays valid as a container - for the duration of invocation of synchronousAddEvent
call, that immediately performs serialization of event from original container + decoration into either ETW/TraceLogging (binary packed in standard TraceLogging format) or ETW/MessagePack. That (transform + IPC) is done right away on user calling thread with no background batching.In case of other Key-Value iterable container, e.g. in case of a map -- there is a need to iterate for each key-value (as you mentioned, in case of batching exporter and `Recordable). I'll illustrate the difference between when the memcpy transform is unavoidable, and where the original container may be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pushing an update to my code PR to explain this further ( +1 benchmark, yay! ), but that won't change the statement made here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely summarized the build options. Few of the options ( HAVE_ABSEIL_VARIANT, HAVE_CPP_STDLIB, HAVE_ABSEIL_VARIANT ) are applicable as build instructions for
opentelemetry-cpp
, do you think we can move them there ( https://github.com/open-telemetry/opentelemetry-cpp/blob/main/INSTALL.md#building-as-standalone-cmake-project ), give it's reference and keep rest of them here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I want to have a separate document created that covers ALL options. Maybe a separate section in
INSTALL.md
as you are suggesting. For this one - I tried to keep it scoped to what is relevant in this concrete usage scenario.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the description for the options, could we add an extra column to the the pros and even cons of using each option to help users to make decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of elaborating on what this option is doing. But maybe in that same description column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few more sentences.
HAVE_CPP_STDLIB
has its own separate detailed design doc here:https://github.com/open-telemetry/opentelemetry-cpp/blob/main/docs/building-with-stdlib.md