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

Adding platform telemetry #2109

Merged
merged 19 commits into from
Oct 20, 2019
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
**[Contributions and Feedback](#contribute)**

**[License](#license)**

**[Data/Telemetry](#Data/Telemetry)**
***
# Key Features
## Run any ONNX model
Expand Down Expand Up @@ -174,3 +176,8 @@ or contact [[email protected]](mailto:[email protected]) with any addi
***
# License
[MIT License](LICENSE)

# Data/Telemetry
Copy link
Contributor

@pranavsharma pranavsharma Oct 16, 2019

Choose a reason for hiding this comment

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

We must call out that it is disabled by default. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to have the DOCS clear that telemetry will be on by default. then make the code obey that as and when we are ready . What I want to avoid is have the docs say that they are off by default, and then when we later turn the code on it's not super clear that you need to go an read the docs again (aka breaking change). in this manner, the docs and "design" are all such that it is clearly messaged telemetry is ON by default. it's a "bug" that we don't have the code behaving per spec yet. In that fashion we can "fix the code" later and all be good. It isnt a breaking change . you like ?


In reply to: 335297148 [](ancestors = 335297148)

Copy link
Contributor

@pranavsharma pranavsharma Oct 17, 2019

Choose a reason for hiding this comment

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

I thought we decided to leave the session option for this to be disabled by default in 1.0 release. The session option config should match the docs, right? Or I'm missing something? #Resolved

Copy link
Contributor

@faxu faxu Oct 17, 2019

Choose a reason for hiding this comment

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

The docs should reflect the current state. To start, it will be OFF by default. When it's out of preview and ready to fully turn on, we will update the documentation to reflect that state.

For the specific documentation, I suggest:

  • main readme page mentions that telemetry collection is implemented in this project and to refer to the detailed data collection page. We don't need to change this documentation when default state changes.
  • data collection page includes privacy policy as well as the documentation on how to enable/disable
  • C API page just links to data collection page. We can still have a section heading, but it will contain nothing but the link to the data collection page. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change the docs to denote it is OFF by default during BETA, and to expect it to ON by default once it graduates from BETA (for transparency)


In reply to: 335777476 [](ancestors = 335777476)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this change like you suggested, just a heads up that now we have C API specific docs embedded in the privacy statement. If we ever have projections on these (like C# or WinRT) we might have to figure out where to put those docs. I had the turn/on and off in the C_API docs since it was specific to the C_API.

Let me know what you think with the new doc layout !


In reply to: 335829030 [](ancestors = 335829030)

Copy link
Contributor

@faxu faxu Oct 17, 2019

Choose a reason for hiding this comment

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

Let's put this above the License #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it above license.


In reply to: 335830799 [](ancestors = 335830799)

This project collects usage data and sends it to Microsoft to help improve our products and services. See the [privacy statement](docs/Privacy.md) for more details.

For more information on telemetry implementation see the [developer guide](docs/C_API.md#Telemetry).
2 changes: 2 additions & 0 deletions cmake/onnxruntime_common.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ set(onnxruntime_common_src_patterns
"${ONNXRUNTIME_ROOT}/core/platform/env.cc"
"${ONNXRUNTIME_ROOT}/core/platform/env_time.h"
"${ONNXRUNTIME_ROOT}/core/platform/env_time.cc"
"${ONNXRUNTIME_ROOT}/core/platform/telemetry.h"
"${ONNXRUNTIME_ROOT}/core/platform/telemetry.cc"
)

if(WIN32)
Expand Down
2 changes: 2 additions & 0 deletions csharp/src/Microsoft.ML.OnnxRuntime/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ public struct OrtApi
public IntPtr GetErrorMessage;
public IntPtr CreateEnv;
public IntPtr CreateEnvWithCustomLogger;
public IntPtr EnableTelemetryEvents;
public IntPtr DisableTelemetryEvents;
public IntPtr CreateSession;
public IntPtr CreateSessionFromArray;
public IntPtr Run;
Expand Down
9 changes: 9 additions & 0 deletions docs/C_API.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,12 @@
The example below shows a sample run using the SqueezeNet model from ONNX model zoo, including dynamically reading model inputs, outputs, shape and type information, as well as running a sample vector and fetching the resulting class probabilities for inspection.

* [../csharp/test/Microsoft.ML.OnnxRuntime.EndToEndTests.Capi/C_Api_Sample.cpp](../csharp/test/Microsoft.ML.OnnxRuntime.EndToEndTests.Capi/C_Api_Sample.cpp)

# Telemetry
This project collects usage data and sends it to Microsoft to help improve our products and services. Note however that no data collection is performed by default when using your private builds

Telemetry is turned on by default when using the Official Builds. This is implemented via 'Platform Telemetry' per vendor platform providers (see telemetry.h).
Copy link
Contributor

@pranavsharma pranavsharma Oct 16, 2019

Choose a reason for hiding this comment

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

Turned "off" by default. Also, is it better to write the documentation in one place and link to it from everywhere else? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check out my comments on the other doc on why i like have the "spec" be ON by default, and the code will catch up later when we are ready (bug level fixes) . as to the docs spread around a bit, i went back and forth on it :) . pros/cons.

  1. other OSS components have multiple docs. having things like a "privacy statment" are nice.
  2. i like the main README being more of a TOC and link off. that way the root/readme doesn't just bloat and bloat with content.
  3. i like the idea of drill down for details. the top level docs have basic ideas. and you get to go down deeper (like the c_api doc) to learn how to use the actual C api in this fashion.

what ya think?


In reply to: 335297643 [](ancestors = 335297643)

Copy link
Contributor

@pranavsharma pranavsharma Oct 17, 2019

Choose a reason for hiding this comment

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

It's ok as long as we don't end up with inconsistent documentation. @faxu can probably review this and give some comments. #Resolved

Copy link
Contributor

@faxu faxu Oct 17, 2019

Choose a reason for hiding this comment

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

as mentioned above, the spec/documentation should reflect the current state for accuracy. that said, the top-level page (main readme) can be general enough to not require updating once default state changes. The intent of that page is to inform about the presence of telemetry-collecting code. All further details on default states can live on the Privacy info page.

The info on how to turn on/off should also be on that Privacy info page, I think. #Resolved


The Windows provider uses the [TraceLogging](https://docs.microsoft.com/en-us/windows/win32/tracelogging/trace-logging-about) API for its implementation.

You can turn this off using the DisableTelemetryEvents() API.
4 changes: 4 additions & 0 deletions docs/Privacy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Data Collection
The software may collect information about you and your use of the software and send it to Microsoft. Microsoft may use this information to provide services and improve our products and services. You may turn off the telemetry as described in the repository. There are also some features in the software that may enable you and Microsoft to collect data from users of your applications. If you use these features, you must comply with applicable law, including providing appropriate notices to users of your applications together with a copy of Microsoft's privacy statement. Our privacy statement is located at https://go.microsoft.com/fwlink/?LinkID=824704. You can learn more about data collection and use in the help documentation and our privacy statement. Your use of the software operates as your consent to these practices.

For more information on telemetry implementation see the [developer guide](C_API.md#Telemetry).
26 changes: 19 additions & 7 deletions include/onnxruntime/core/common/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ using common::Status;
static_cast<void>(fn)

std::vector<std::string> GetStackTrace();
// these is a helper function that gets defined by platform/Telemetry
void LogRuntimeError(uint32_t session_id, const common::Status& status, const char* file,
const char* function, uint32_t line);

// __PRETTY_FUNCTION__ isn't a macro on gcc, so use a check for _MSC_VER
// so we only define it as one for MSVC
Expand Down Expand Up @@ -137,16 +140,25 @@ std::vector<std::string> GetStackTrace();
ORT_DISALLOW_COPY_AND_ASSIGNMENT(TypeName); \
ORT_DISALLOW_MOVE(TypeName)

#define ORT_RETURN_IF_ERROR(expr) \
do { \
auto _status = (expr); \
if ((!_status.IsOK())) return _status; \
#define ORT_RETURN_IF_ERROR_SESSIONID(expr, session_id) \
do { \
auto _status = (expr); \
if ((!_status.IsOK())) { \
::onnxruntime::LogRuntimeError(session_id, _status, __FILE__, __FUNCTION__, __LINE__); \
return _status; \
} \
} while (0)

#define ORT_RETURN_IF_ERROR_SESSIONID_(expr) ORT_RETURN_IF_ERROR_SESSIONID(expr, session_id_)
#define ORT_RETURN_IF_ERROR(expr) ORT_RETURN_IF_ERROR_SESSIONID(expr, 0)

#define ORT_THROW_IF_ERROR(expr) \
Copy link
Contributor

@pranavsharma pranavsharma Oct 16, 2019

Choose a reason for hiding this comment

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

Do we plan to send events for cases when we just do ORT_THROW? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. I first coded ORT_RETURN_IF and ORT_THROW_IF . it covers most all code paths, but not all. how about i file a tracking item in a second PR to catch all of the macros?


In reply to: 335302117 [](ancestors = 335302117)

Copy link
Contributor

@pranavsharma pranavsharma Oct 17, 2019

Choose a reason for hiding this comment

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

Sure, that's fine. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added as a work item


In reply to: 335777787 [](ancestors = 335777787)

do { \
auto _status = (expr); \
if ((!_status.IsOK())) ORT_THROW(_status); \
if ((!_status.IsOK())) { \
::onnxruntime::LogRuntimeError(0, _status, __FILE__, __FUNCTION__, __LINE__); \
ORT_THROW(_status); \
} \
} while (0)

// use this macro when cannot early return
Expand All @@ -164,8 +176,8 @@ std::vector<std::string> GetStackTrace();
#define GSL_SUPPRESS(tag)
#endif

inline void MakeStringInternal(std::ostringstream& /*ss*/) noexcept {
}
inline void MakeStringInternal(std::ostringstream& /*ss*/) noexcept {
}

template <typename T>
inline void MakeStringInternal(std::ostringstream& ss, const T& t) noexcept {
Expand Down
6 changes: 6 additions & 0 deletions include/onnxruntime/core/common/version.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#pragma once

#define ONNXRUNTIME_VERSION_STRING "1.0"
4 changes: 4 additions & 0 deletions include/onnxruntime/core/session/onnxruntime_c_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,10 @@ struct OrtApi {
_In_ const char* logid,
_Outptr_ OrtEnv** out)NO_EXCEPTION;

// Platform telemetry events are on by default since they are lightweight. You can manually turn them off.
OrtStatus*(ORT_API_CALL* EnableTelemetryEvents)(_In_ const OrtEnv* env)NO_EXCEPTION;
OrtStatus*(ORT_API_CALL* DisableTelemetryEvents)(_In_ const OrtEnv* env)NO_EXCEPTION;

// TODO: document the path separator convention? '/' vs '\'
// TODO: should specify the access characteristics of model_path. Is this read only during the
// execution of OrtCreateSession, or does the OrtSession retain a handle to the file/directory
Expand Down
3 changes: 3 additions & 0 deletions include/onnxruntime/core/session/onnxruntime_cxx_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ struct Env : Base<OrtEnv> {
Env(OrtLoggingLevel default_logging_level, const char* logid, OrtLoggingFunction logging_function, void* logger_param);
explicit Env(OrtEnv* p) : Base<OrtEnv>{p} {}

Env& EnableTelemetryEvents();
Env& DisableTelemetryEvents();

static const OrtApi* s_api;
};

Expand Down
10 changes: 10 additions & 0 deletions include/onnxruntime/core/session/onnxruntime_cxx_inline.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,16 @@ inline Env::Env(OrtLoggingLevel default_warning_level, const char* logid, OrtLog
ThrowOnError(g_api->CreateEnvWithCustomLogger(logging_function, logger_param, default_warning_level, logid, &p_));
}

inline Env& Env::EnableTelemetryEvents() {
ThrowOnError(g_api->EnableTelemetryEvents(p_));
return *this;
}

inline Env& Env::DisableTelemetryEvents() {
ThrowOnError(g_api->DisableTelemetryEvents(p_));
return *this;
}

inline CustomOpDomain::CustomOpDomain(const char* domain) {
ThrowOnError(g_api->CreateCustomOpDomain(domain, &p_));
}
Expand Down
4 changes: 4 additions & 0 deletions onnxruntime/core/platform/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ limitations under the License.
#include "core/common/common.h"
#include "core/framework/callback.h"
#include "core/platform/env_time.h"
#include "core/platform/telemetry.h"

#ifndef _WIN32
#include <sys/types.h>
Expand Down Expand Up @@ -133,6 +134,9 @@ class Env {
// returns the name that LoadDynamicLibrary() can use
virtual std::string FormatLibraryFileName(const std::string& name, const std::string& version) const = 0;

// \brief returns a provider that will handle telemetry on the current platform
virtual const Telemetry& GetTelemetryProvider() const = 0;

protected:
Env();

Expand Down
6 changes: 6 additions & 0 deletions onnxruntime/core/platform/posix/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,14 @@ class PosixEnv : public Env {
return filename;
}

// \brief returns a provider that will handle telemetry on the current platform
const Telemetry& GetTelemetryProvider() const override {
return telemetryProvider_;
Copy link
Contributor

@pranavsharma pranavsharma Oct 16, 2019

Choose a reason for hiding this comment

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

telemetry_provider_ #Resolved

}

private:
PosixEnv() = default;
Telemetry telemetryProvider_;
};

} // namespace
Expand Down
59 changes: 59 additions & 0 deletions onnxruntime/core/platform/telemetry.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#include "core/platform/telemetry.h"
#include "core/platform/env.h"

namespace onnxruntime {

void LogRuntimeError(uint32_t sessionId, const common::Status& status, const char* file,
const char* function, uint32_t line)
{
const Env& env = Env::Default();
env.GetTelemetryProvider().LogRuntimeError(sessionId, status, file, function, line);
}

void Telemetry::EnableTelemetryEvents() const {
}

void Telemetry::DisableTelemetryEvents() const {
}

void Telemetry::LogProcessInfo() const {
}

void Telemetry::LogSessionCreation(uint32_t session_id, int64_t ir_version, const std::string& model_producer_name,
const std::string& model_producer_version, const std::string& model_domain,
const std::unordered_map<std::string, int>& domain_to_version_map,
const std::string& model_graph_name,
const std::unordered_map<std::string, std::string>& model_metadata,
const std::string& loadedFrom, const std::vector<std::string>& execution_provider_ids) const {
ORT_UNUSED_PARAMETER(session_id);
ORT_UNUSED_PARAMETER(ir_version);
ORT_UNUSED_PARAMETER(model_producer_name);
ORT_UNUSED_PARAMETER(model_producer_version);
ORT_UNUSED_PARAMETER(model_domain);
ORT_UNUSED_PARAMETER(domain_to_version_map);
ORT_UNUSED_PARAMETER(model_graph_name);
ORT_UNUSED_PARAMETER(model_metadata);
ORT_UNUSED_PARAMETER(loadedFrom);
ORT_UNUSED_PARAMETER(execution_provider_ids);
}

void Telemetry::LogRuntimeError(uint32_t session_id, const common::Status& status, const char* file,
const char* function, uint32_t line) const {
ORT_UNUSED_PARAMETER(session_id);
ORT_UNUSED_PARAMETER(status);
ORT_UNUSED_PARAMETER(file);
ORT_UNUSED_PARAMETER(function);
ORT_UNUSED_PARAMETER(line);
}

void Telemetry::LogRuntimePerf(uint32_t session_id, uint32_t total_runs_since_last, int64_t total_run_duration_since_last) const {
ORT_UNUSED_PARAMETER(session_id);
ORT_UNUSED_PARAMETER(total_runs_since_last);
ORT_UNUSED_PARAMETER(total_run_duration_since_last);
}

} // namespace onnxruntime

55 changes: 55 additions & 0 deletions onnxruntime/core/platform/telemetry.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#pragma once

#include <string>
#include <vector>
#include <unordered_map>

#include "core/common/status.h"
#include "core/common/common.h"

namespace onnxruntime {

/**
* Configuration information for a session.
* An interface used by the onnxruntime implementation to
* access operating system functionality for telemetry
*
* look at env.h and the Env objection which is the activation factory
* for telemetry instances
*
* All Telemetry implementations are safe for concurrent access from
* multiple threads without any external synchronization.
*/
class Telemetry {
public:
// don't create these, use Env::GetTelemetryProvider() instead
// this constructor is made public so that other platform Env providers can
// use this base class as a "stub" implementation
Telemetry() = default;
virtual ~Telemetry() = default;

virtual void EnableTelemetryEvents() const;
virtual void DisableTelemetryEvents() const;

virtual void LogProcessInfo() const;

virtual void LogSessionCreation(uint32_t session_id, int64_t ir_version, const std::string& model_producer_name,
const std::string& model_producer_version, const std::string& model_domain,
const std::unordered_map<std::string, int>& domain_to_version_map,
const std::string& model_graph_name,
const std::unordered_map<std::string, std::string>& model_metadata,
const std::string& loadedFrom, const std::vector<std::string>& execution_provider_ids) const;

virtual void LogRuntimeError(uint32_t session_id, const common::Status& status, const char* file,
const char* function, uint32_t line) const;

virtual void LogRuntimePerf(uint32_t session_id, uint32_t total_runs_since_last, int64_t total_run_duration_since_last) const;

private:
ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE(Telemetry);
};

} // namespace onnxruntime
8 changes: 7 additions & 1 deletion onnxruntime/core/platform/windows/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ limitations under the License.

#include "core/common/logging/logging.h"
#include "core/platform/env.h"
#include "core/platform/windows/telemetry.h"

namespace onnxruntime {

Expand Down Expand Up @@ -211,6 +212,11 @@ class WindowsEnv : public Env {
ORT_NOT_IMPLEMENTED(__FUNCTION__, " is not implemented");
}

// \brief returns a provider that will handle telemetry on the current platform
const Telemetry& GetTelemetryProvider() const override {
return telemetry_provider_;
}

private:
WindowsEnv()
: GetSystemTimePreciseAsFileTime_(nullptr) {
Expand All @@ -228,8 +234,8 @@ class WindowsEnv : public Env {

typedef VOID(WINAPI* FnGetSystemTimePreciseAsFileTime)(LPFILETIME);
FnGetSystemTimePreciseAsFileTime GetSystemTimePreciseAsFileTime_;
WindowsTelemetry telemetry_provider_;
};

} // namespace

#if defined(PLATFORM_WINDOWS)
Expand Down
Loading