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 |
maxgolov
changed the title
maxgolov
added
area:exporter
spec-compliance
| evtFmt = ETWProvider::EventFormat::ETW_MANIFEST; | ||
| break; | ||
|
|
||
| default: |
There was a problem hiding this comment.
Assert here instead of ignore silently?
There was a problem hiding this comment.
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.
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.
Why report kUnset as success, for other exporter like Zipkin, the status is not reported for kUnset.
There was a problem hiding this comment.
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.
nit: rename the second argument as span?
There was a problem hiding this comment.
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.
Cannot we read this from the span instead of using a hard-code kind?
There was a problem hiding this comment.
Let me get back to you on this.
| break; | ||
|
|
||
| case CONST_HASHCODE(TLD): | ||
| // nobrk |
| evtFmt = ETWProvider::EventFormat::ETW_MANIFEST; | ||
| break; | ||
|
|
||
| default: |
There was a problem hiding this comment.
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
memcpyof originalconst char *buffer intostd::string-owning object because original buffer MAY refer to temporary. We can tune this later on if this extramemcpyever becomes an issue. FavoringAPI usabilityandstabilityover perf at this point. Without this fix theconst char *andstring literalparameters 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
5054when compiling with Visual Studio 2019. This warning is triggered in "external" projectTraceLoggingDynamic.hheader 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_TLDFeature Gate: we now always includeTraceLoggingDynamic.hthat'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_PARAMETERmacro 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_PARAMETERshould 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
SetStatusAPI.Timestamps: add option
HAVE_FIELD_TIMEthat 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
TracerProviderConfigurationconcepts,HAVE_FIELD_TIME,ETW_FIELD_ENDTTIME, etc. in a separate "documentation-only" examples + PR.