-
Notifications
You must be signed in to change notification settings - Fork 411
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 warnings clean-up, compliance, implementing 2 TODOs #702
Conversation
Codecov Report
@@ Coverage Diff @@
## main #702 +/- ##
=======================================
Coverage 94.76% 94.76%
=======================================
Files 217 217
Lines 9929 9929
=======================================
Hits 9409 9409
Misses 520 520 |
evtFmt = ETWProvider::EventFormat::ETW_MANIFEST; | ||
break; | ||
|
||
default: |
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.
Assert here instead of ignore silently?
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.
Presently it is falling back to default (ETW
). I am wondering if we should add a comment here that maybe we should log a warning. Similar to what is done when nullptr
is passed as name to GetTracer
, i.e. it would probably be in spirit of the rest of API to ignore with some internal trace warning.. When we have a system for actually logging the warnings, we can log a warning. I don't think we should assert.
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.
We can also return noop tracer if the encoding is incorrect/not-supporter.
{ | ||
/* Should we avoid populating this extra field if status is unset? */ | ||
if ((status_code_ == trace::StatusCode::kUnset) || (status_code_ == trace::StatusCode::kOk)) | ||
{ |
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.
Why report kUnset
as success, for other exporter like Zipkin, the status is not reported for kUnset
.
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.
Okay, I will change that. I was not sure.
@@ -1033,44 +1147,8 @@ class TracerProvider : public trace::TracerProvider | |||
nostd::shared_ptr<trace::Tracer> GetTracer(nostd::string_view name, | |||
nostd::string_view args = "") override |
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.
nit: rename the second argument as span
?
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 think the second argument should be library_version
most likely? The first one is instrument
or instrumentation
name, and the second argument remains unused.. I guess can rename it? Or I can omit the name of it. I was previously thinking to pass some additional arguments to tracer, using that second parameter. That is why it was named that way. But since this was not in alignment with the API spec - I stopped using it. This argument does not make sense for statically linked header-only ETW TracerProvider, that practically implements its own SDK flavor with just API surface.
// Duration of Span in milliseconds | ||
evt[ETW_FIELD_DURATION] = endTimeMs - startTimeMs; | ||
// Presently we assume that all spans are server spans | ||
evt[ETW_FIELD_SPAN_KIND] = uint32_t(trace::SpanKind::kServer); |
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.
Cannot we read this from the span instead of using a hard-code kind?
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.
Let me get back to you on this.
break; | ||
|
||
case CONST_HASHCODE(TLD): | ||
// nobrk |
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.
nit: remove this comment
evtFmt = ETWProvider::EventFormat::ETW_MANIFEST; | ||
break; | ||
|
||
default: |
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.
We can also return noop tracer if the encoding is incorrect/not-supporter.
@maxgolov I have merged this PR. There were few open comments, if it needs changes please raise separate PR for that. |
@lalitb @ThomsonTan - absolutely. I'll address these minor comments when I add support for Span Links #649 |
This change is isolated to ETW exporter. Mostly compliance and warnings fixes.
Changes
There are no significant changes, only bugfix, plus adding missing features / resolving TODOs.
List of fixes:
PropertyValue Container: perform
memcpy
of originalconst char *
buffer intostd::string
-owning object because original buffer MAY refer to temporary. We can tune this later on if this extramemcpy
ever becomes an issue. FavoringAPI usability
andstability
over perf at this point. Without this fix theconst char *
andstring literal
parameters may not be processed correctly in some scenarios.arrays
- are still not supported yet by ETW exporter. AddingUNREFERENCED_PARAMETER(vec);
to avoid the compilation L4 warning that may be a problem for envs where warnings are treated as errors. Will address the arrays support as feature work in a separate PR.disable warning
5054
when compiling with Visual Studio 2019. This warning is triggered in "external" projectTraceLoggingDynamic.h
header that I am not going to touch. The warning itself is benign and unique to 2019. Older compilers do not emit it. It is a benign conformance warning that has rather low practical value in our use case.remove aliases for
using Properties = exporter::etw::Properties; using PropertyType = exporter::etw::PropertyType;
- redeclaring these in another namespace was causing ambiguity for Visual Studio 2015 - Update 2. This does not functionally change anything, and it was not an issue for 2017 (C++14
) and 2019 (C++17
) that we have in the build loop. Thus, it wasn't immediately caught.remove
HAVE_TLD
Feature Gate: we now always includeTraceLoggingDynamic.h
that's been open-sourced and merged in another PR Add TraceLogging Dynamic header for ETW exporter (optional MIT-licensed header) #689 (Yay!) . So the option becomes irrelevant.Other L4 warnings clean-up: when compiling without
HAVE_MSGPACK
- since (see above), TLD is always included, we addUNREFERENCED_PARAMETER
macro to resolve L4 warnings. This code is presently Windows-only. And the macro is a standard macro for Visual Studio. Since the same code may be compiled with / without the feature, we have to preserve the parameter names. I could have usedstd::ignore
, butUNREFERENCED_PARAMETER
should be Okay - within the constraints of a typical Windows-only code (which ETW exporter is).GetTracer API : second parameter used to specify the encoding type (MsgPack or TraceLoggingDynamic). Since this was NOT in alignment with spec - which suggests the second param is a
library_version
, I'm moving the encoding type to ETW Tracer Provider configuration option calledTracerProviderConfiguration::encoding
. That means now all Tracers obtained from a given ETW provider instance - use the same binary encoding (TLD as default). In most cases - since TLD is the better, preferred encoding anyways, we do not sacrifice too much (if anything). Whatever it takes to keep the API surface in alignment with the spec! Second parameter in case of ETW exporter is optional (ignored completely) since ETW exporter isheader-only
. We can discuss how to implement that parameter, should there be ever the need for inclusion of ETW exporter in a shared library. For now - this is unnecessary. All of it is lean and instrumentation may get compiled straight into customer code with no external DLL.Status API: implement
SetStatus
API.Timestamps: add option
HAVE_FIELD_TIME
that allows to stamp an additional time field encoded as text (alongside with standard ETW time that comes at envelope level); plus make time conversion routines static inline, don't export them; future-proof ISO8601 time formatting routine to include up to 6 digits after the dot instead of 3. This may help in a flow where a backend is expecting a format with 6 digits precision.I will describe the new compile- and runtime- options, such as ETW
TracerProviderConfiguration
concepts,HAVE_FIELD_TIME
,ETW_FIELD_ENDTTIME
, etc. in a separate "documentation-only" examples + PR.