[Proposal] Make table name configurable in OpenTelemetry C++ ETW exporter

[ThomsonTan]

In the current OpenTelemetry ETW Log exporter, the attribute for table name is hard-coded to "Log" (see below 2 links for more details). The table name is useful for grouping events in the backend, especially for the Logging signal. Based on that the ETW exporter is header-only, this table name can only be changed by patching the source code and recompiling the source code which could be inconvenient because the same source file can only have one table name defined.

opentelemetry-cpp/exporters/etw/include/opentelemetry/exporters/etw/etw_fields.h

Line 128 in 78d488c

# define ETW_VALUE_LOG "Log" /* ETW event name for Log */

opentelemetry-cpp/exporters/etw/include/opentelemetry/exporters/etw/etw_logger.h

Line 274 in 78d488c

evt[ETW_FIELD_NAME] = ETW_VALUE_LOG;

Here are the 2 proposals about making this table name configurable at launch/runtime.

1. Reuse the library_name as table name

OpenTelemetry API has library name in GetLogger, but it is not used in ETW exporter (see here). So library_name can be mapped to table name under explicit enlightenment in the provider option. The config code looks like below.

  opentelemetry::exporter::etw::TelemetryProviderOptions options {
    {"libraryNameAsTableName", true}
  };
  opentelemetry::exporter::etw::LoggerProvider logger_provider(options);
  auto logger = logger_provider.GetLogger(providerName, "my_table_name");

Pros

• It doesn't break existing code for upgrading with the explicit enlightenment.
• No extra perf cost on logging besides configuration option which is only for launch time.
• It is consistent with .NET which defines the mapping in exporter configuration.

Cons

• The library_name will not be able to be used for other mapping once this is enabled

2. Pass pre-defined attribute as table name for each Log call

We can also define a unique attribute to set the table as below.

  logger->Info("Hello World!", common::MakeAttributes{{"unique_table_name_key", "my_event_name"}});

Pros

• No need to take out library_name from unused list.

Cons

• Extra runtime cost for each Log call.
  • Extra memory allocated for the key value pairs struct.
  • Extra CPU time on initialize the key value pairs container, and also query the unique key to extract table name in the container.
• The new convention of defining the unique key for table name causes inconsistency with other languages like .NET Geneva exporter, which does this in configuration option.

I prefer the option 1 above, let me know your suggestion or other options.

[lalitb]

Nicely summarised. Another option could be to utilize the existing support of EventId in the Log API. So user can provide that during logging:

 EventId event_id(12345);
logger->Info(event_id, "HelloWorld!", optional_attributes);

And, to be consistent with .Net implementation, our configuration can have the mapping between the EventId::id and event name, along with the default event name to use if the EventId is not provided during log:

    std::unordered_map<int, std::string> event_map = {
        {12345, "event1"},
        {4444, "event2"},
    };
    constexpr const char* DEFAULT_EVENT_NAME = "default_event";

The hashmap access would be O(1), so hot path performance won't be affected.
If DEFAULT_EVENT_NAME is not provided, we can use the macro LOG_EVENT_NAME (?) for the event name.
name field is optional in EventId, so we can ignore that.

This looks to be least disruptive to me.

[reyang]

It's unclear to me if we are talking about event name or table name, I saw both being mentioned in the discussion so far. @ThomsonTan would you clarity?

[lalitb]

Sorry, I mentioned the table name. It should have been event name. Updated now.

[ThomsonTan]

Updated both title and the proposal and fixed it. I meant table name which is internally set as ETW event name. As the internal ETW event name cause confuse with the event name in our logging API, I change it to table name for consistency.

It's unclear to me if we are talking about event name or table name, I saw both being mentioned in the discussion so far. @ThomsonTan would you clarity?

[reyang]

Both options will require the users to change instrumentation if they want to change the destination table name, which seems to go against the separation of API and SDK - for example, as a user, I might want to configure logs generated from other libraries, while my own code does not use the logging API at all.

[reyang]

OpenTelemetry API has library name in GetLogger

BTW, why is this called "library name" instead of "name"? (instrumentation scope) Sounds a bit confusing to me. Is this something that we should fix?

[ThomsonTan]

Both options will require the users to change instrumentation if they want to change the destination table name, which seems to go against the separation of API and SDK - for example, as a user, I might want to configure logs generated from other libraries, while my own code does not use the logging API at all.

For option 1 and 2, user code change is required for the both the exporter configuration and Log instrumentation. One reason for this is I don't want to break or change behavior of existing instrumented code. For the instrumentation log call, this is one time change, and we can make it a map from name to table name, then if the user decide to map the log to a different table, only the ETW exporter configuration needs to be updated, no change needed for the log call.

Configure library logging behavior in the main app is not supported for now with the current static linking, because all the SDK/exporter status are local to the module (.exe or .dll) and are not supposed to be pass across the binary boundary. Another reason for this is that the DI (dependency injection) pattern is not common in C++ Logging library, so we cannot pass the SDK/exporter status to the third party library like the .NET does (ILogggerFactory or ILogger are created in the app and passed to the third party library for logging).

Change the header-only design of ETW exporter and build it into DLL could make it work, as the exporter configuration will be global to the whole process and apply to all the libraries then.

[ThomsonTan]

OpenTelemetry API has library name in GetLogger

BTW, why is this called "library name" instead of "name"? (instrumentation scope) Sounds a bit confusing to me. Is this something that we should fix?

Yes, created issue #2689 for tracking.