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.

Sign up for GitHub

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

Jump to bottom

ETW Tracer exporter #376

Merged
merged 32 commits into from
Jan 6, 2021
Merged

ETW Tracer exporter #376

merged 32 commits into from
Jan 6, 2021

Conversation

mishal23
Copy link
Contributor

@mishal23 mishal23 commented Oct 27, 2020 •
edited
Loading

The PR is the initial commit for the tracer exporter for ETW from the example written by @maxgolov. Issue #326

This PR adds:

  • Headers for the ETW Tracer exporter
  • Unit Tests for the classes

Edit 1 (25/11/2020):

code has been updated as per the specification.

-- cc @maxgolov

@mishal23 mishal23 requested a review from a team October 27, 2020 05:04
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 27, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Oct 27, 2020 •
edited
Loading

Codecov Report

Merging #376 (a373b98) into master (614d3c0) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   94.36%   94.37%   +0.01%     
==========================================
  Files         189      189              
  Lines        8410     8410              
==========================================
+ Hits         7936     7937       +1     
+ Misses        474      473       -1
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/trace/processor.h 100.00% <ø> (ø)
...include/opentelemetry/sdk/trace/simple_processor.h 88.23% <ø> (ø)
sdk/src/logs/batch_log_processor.cc 95.06% <0.00%> (+1.23%) ⬆️

maxgolov
maxgolov reviewed Oct 30, 2020
View reviewed changes
.gitmodules Outdated Show resolved Hide resolved
maxgolov
maxgolov reviewed Nov 6, 2020
View reviewed changes
ci/do_ci.sh Outdated Show resolved Hide resolved
@maxgolov
Copy link
Contributor

maxgolov commented Dec 31, 2020 •
edited
Loading

@pyohannes - I resolved your concern re. EventWriteString by using EventWrite. I verified that EventWrite works and respects the zero words in the middle of string. Fixes #483

@maxgolov maxgolov linked an issue Dec 31, 2020 that may be closed by this pull request
Possible event loss in ETW exporter #483
Closed
pyohannes
pyohannes reviewed Dec 31, 2020
View reviewed changes
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I'm giving this a first review (sorry for chiming in so late in the game). I put some thoughts and questions inline.

Ideally we'd get rid of the (30) TODOs, but I think it's planned to do that after merging this PR, to not prolong the process further.

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h

/// <summary>
/// stream::Tracer class that allows to send spans to ETW
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we adhere to the established format for doc comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

By default, the stub generation in Visual Studio for C++ is set to XML Doc Comments (///). I agree with your suggestion, but I'd like to clean it up in a separate PR, which will have no code changes other than cleaning up the comments from /// to /**.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doc strings should be brought in line with existing standards.

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h
{
// TODO: support attributes
UNREFERENCED_PARAMETER(attributes);
UNREFERENCED_PARAMETER(links);
Copy link
Contributor

Choose a reason for hiding this comment

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

The preferred way to avoid warning for unreferenced parameters is to use then unnamed:

virtual nostd::shared_ptr<trace::Span> StartSpan(
      nostd::string_view name,
      const common::KeyValueIterable & /*attributes */,
      const trace::SpanContextKeyValueIterable & /*links */,
      const trace::StartSpanOptions &options = {}) noexcept override

Copy link
Contributor

@maxgolov maxgolov Dec 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This will be a problem for doxygen: https://stackoverflow.com/questions/55421894/how-do-i-doxygen-document-unnamed-parameters-of-a-function ... Since this module is Windows-specific, I believe UNREFERENCED_PARAMETER would be appropriate, esp. if we want to run doxygen on that code. Support for both parameters (attributes, links) will be added in a separate PR.. Thus, once these would be actually used, we'd safely remove the UNREFERENCED_PARAMETER macro (in a separate PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point.

We follow the pattern with unnamed parameters in other places, because it's true to ISO CPP standards. I also think that in most cases implementations of virtual methods in derived classes don't necessarily need documentation.

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/etw_provider_exporter.h Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/etw_tracer_exporter.h Outdated Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/uuid.hpp Outdated Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/etw_provider_exporter.h Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/etw_data.h Show resolved Hide resolved
exporters/etw/src/etw_tracer_exporter.cc Show resolved Hide resolved
exporters/etw/src/etw_tracer_exporter.cc Outdated
@@ -0,0 +1,7 @@
#ifdef _WIN32

# define HAVE_NO_TLD
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be defined here, when we also have it defined in a CMakeLists.txt file? Otherwise this will always override what comes from CMake.

Copy link
Contributor

@maxgolov maxgolov Dec 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is kept for now to keep Bazel build happy. This will be removed once we OSS the TraceLoggingDynamic.h header. Equivalent TLD functionality is available in Go, for example, here: https://godoc.org/github.com/microsoft/go-winio/pkg/etw .. so we will democratize the proper full support of ETW dynamic C++ events and this define will go away. Issue with Bazel is we are trying to avoid conditional compilation to full extent.... Path forward would be to completely eliminate HAVE_NO_TLD feature gate.

pyohannes
pyohannes approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Approved, as @maxgolov prefers to apply changes in different PRs. I tried to summarize open issues below in separate comments.

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/etw_data.h

/// <summary>
/// stream::Tracer class that allows to send spans to ETW
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc strings should be brought in line with existing standards.

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h
/// <summary>
/// Provider Id (Name or GUID)
/// </summary>
std::string provId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually for class data members we use lower case with underscores and a trailing underscore. This should be brought in line with existing standards for all occurrences.

https://google.github.io/styleguide/cppguide.html#Variable_Comments

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h
{
// TODO: support attributes
UNREFERENCED_PARAMETER(attributes);
UNREFERENCED_PARAMETER(links);
Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point.

We follow the pattern with unnamed parameters in other places, because it's true to ISO CPP standards. I also think that in most cases implementations of virtual methods in derived classes don't necessarily need documentation.

exporters/etw/include/opentelemetry/exporters/etw/etw_data.h Show resolved Hide resolved
exporters/etw/include/opentelemetry/exporters/etw/etw_data.h
break;
}
#pragma warning(pop)
return nostd::shared_ptr<trace::Tracer>{new (std::nothrow) Tracer(*this, name, evtFmt)};
Copy link
Contributor

Choose a reason for hiding this comment

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

I also just realized that the tracer name is used as ETW provider name. This is broken, for the same reasons given above.

exporters/etw/include/opentelemetry/exporters/etw/etw_provider_exporter.h
}

/* clang-format off */
nlohmann::json jObj =
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic for creating the JSON representation of a span should be refactored into its own method and unit tested.

exporters/etw/include/opentelemetry/exporters/etw/etw_provider_exporter.h
int64_t eventKBSize = (eventByteSize + 1024 - 1) / 1024;
// bool isLargeEvent = eventKBSize >= LargeEventSizeKB;

// TODO: extract
Copy link
Contributor

Choose a reason for hiding this comment

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

All TODO items need to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

exporters/etw/test/etw_tracer_test.cc

EXPECT_NO_THROW(span->AddEvent(eventName, event));
EXPECT_NO_THROW(span->End());
EXPECT_NO_THROW(tracer->CloseWithMicroseconds(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have an example application illustrating the usage of this exporter, ideally with some instructions on how to receive ETW events. This would help people to use, test and verify the functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will commit the ETW listener (written in C#) to contrib repo. Maybe with example how to use this exporter as forward-channel to C#, that may itself then use OpenTelemetry C# to upload the incoming events.

exporters/etw/include/opentelemetry/exporters/etw/etw_provider_exporter.h Show resolved Hide resolved
@lalitb
Copy link
Member

lalitb commented Jan 6, 2021

Approved, as @maxgolov prefers to apply changes in different PRs. I tried to summarize open issues below in separate comments.

Thanks for reviewing it @pyohannes - I am merging this PR and @maxgolov agreed to fix them in separate incremental PRs

@lalitb lalitb merged commit e8ef7ea into open-telemetry:master Jan 6, 2021
@maxgolov maxgolov mentioned this pull request Jan 7, 2021
Closed
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Jan 8, 2021
@mishal23 @kxyr
ETW Tracer exporter (open-telemetry#376)
2922733
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxgolov maxgolov maxgolov approved these changes

@pyohannes pyohannes pyohannes approved these changes

@ThomsonTan ThomsonTan ThomsonTan approved these changes

@jsuereth jsuereth jsuereth approved these changes

@lalitb lalitb Awaiting requested review from lalitb

Assignees
No one assigned
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible event loss in ETW exporter
7 participants
@mishal23 @maxgolov @alolita @lalitb @jsuereth @ThomsonTan @pyohannes