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

OTLP Exporter: Conditionally compile experimental log features #4762

Closed
wants to merge 7 commits into from
Closed

OTLP Exporter: Conditionally compile experimental log features #4762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c3188c
Conditionally compile experimental log features
alanwest Aug 11, 2023
96a280b
Stylecops
alanwest Aug 11, 2023
0a5669d
Stylecops
alanwest Aug 11, 2023
2dea303
Stylecops
alanwest Aug 11, 2023
5b00c21
Merge remote-tracking branch 'upstream/main' into alanwest/ilogger-in…
alanwest Aug 11, 2023
6dfcefb
Document experimental features
alanwest Aug 11, 2023
10374f4
Update readme
alanwest Aug 11, 2023
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
17 changes: 17 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,23 @@ extension scenarios:
* Building a custom sampler for
[traces](./docs/trace/extending-the-sdk/README.md#sampler).

## Experimental Features

Copy link
Member

@vishweshbankwar vishweshbankwar Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Experimental features are enabled in following ways.

Features that require use of public APIs
Public APIs that are available on experimental basis will have a clear API documentation specifying that.
(Example would be great here)

Features that provide new functionality or changes the internal behavior without involvement of public APIs
Such features can be used by opting in via environment variables (e.g., OTEL_DOTNET_EXPERIMENTAL_SOME_FEATURE)

NOTE: Both the ways will only be available in pre-release versions of the libraries. Stable versions will not include the ability to opt-in into experimental features. Users opted in to use experimental features may face build time errors or runtime behavior change if they upgrade to stable version.

Copy link
Contributor

@utpilla utpilla Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

After reviewing this again, I feel that we should have this clear demarcation of which kind of experimental features get offered in what ways. I agree with the above comment made by @vishweshbankwar that:

  • any experimental feature that involves public API changes should be offered using the compiler attribute approach
  • any purely internal behavior change should be offered using the OTEL_DOTNET_EXPERIMENTAL_SOME_FEATURE env vars (I would be opposed to offering them using the compiler attribute feature; I can talk more about my reasons if needed)

I would like to update the note to allow for purely internal behavior changes to be allowed to be shipped as part of the stable release. These are opt-in experimental features. There's no good reason to exclude them from stable releases.

NOTE: Both the ways will only be available in pre-release versions of the libraries. Stable versions will not include the ability to opt-in into experimental features. Users who opted in to use experimental features may face build time errors or runtime behavior change if they upgrade to stable version. Internal behavioral changes offered as part of feature flags maybe included in the stable versions as well.

@alanwest To your concern of the attributes' discussion blocking progress, I'd be okay with commenting out the code in the OTLP Exporter for exporting the three attributes for now and address it after this PR is merged.

Various experimental features are available by the components of OpenTelemetry
.NET.

Experimental features are enabled in a variety of different ways. Some features
are only available in pre-release builds while others may be available in
stable builds gated by a feature flag. Feature flags are implemented using
environment variables (e.g., `OTEL_DOTNET_EXPERIMENTAL_SOME_FEATURE`).

Experimental features are subject to breaking changes. Typically, they are
works-in-progress that we intend to eventually provide in a stable release.
However, this is not guaranteed and they may be removed entirely.

The README of a each component will contain information for any available
experimental features and how to use them.

## Contributing

See [CONTRIBUTING.md](CONTRIBUTING.md)
Expand Down
31 changes: 17 additions & 14 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ exporter.
[specification](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.23.0/specification/metrics/sdk_exporters/otlp.md#additional-configuration).
([#4667](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4667))

* **[Experimental feature](./README.md#experimental-features)**
Copy link
Member

Choose a reason for hiding this comment

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

@alanwest The main README lists multiple ways to do experimental features and then this links to that. Users won't know which type of experimental feature style this happens to use (pre-release build in this case). Could we write the main README with sections on the different types so we could link to an anchor from here? Or some other way you can come up with to make it clear 😄

Copy link
Member

Choose a reason for hiding this comment

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

+1
It would be good to first clarify how we are categorizing experimental features here:

  1. Experimental features offered via public APIs.
  2. Experimental features offered via feature flag that change the internal code behavior.

Also, to me this is not a experimental feature just yet as I do not have a way to opt-in/out :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The main README lists multiple ways to do experimental features and then this links to that.

To clarify, this does not link to the main readme. Did you see the OTLP exporter readme? It documents which style this feature is provided by.

Also, to me this is not a experimental feature just yet as I do not have a way to opt-in/out :)

It's experimental in that it's not gonna be in the next stable release. I've attempted to document things as they will be when this PR merges to main. If in a future PR, someone introduces a feature flag or something for this, then the documentation will need to be updated.

I'm open to making things more clear since that's my main goal with this documentation, but please make a github suggestion proposing different language or formatting, so I can be clear what you find unclear 😄.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, this does not link to the main readme. Did you see the OTLP exporter readme? It documents which style this feature is provided by.

Ah sorry my bad! Looks good.

Copy link
Member

Choose a reason for hiding this comment

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

Something like https://github.com/open-telemetry/opentelemetry-dotnet/pull/4762/files#r1291758235?

Exporting the following fields from `LogRecord` is now considered
an experimental feature: `CategoryName`, `EventId`, and `Exception`.
Refer to the documentation for more details.

## 1.6.0-alpha.1

Released 2023-Jul-12
Expand All @@ -34,20 +39,18 @@ Released 2023-Jul-12
for more details.
([#4647](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4647))

* **Experimental (pre-release builds only):**

* Note: See
[#4735](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4735)
for the introduction of experimental api support.

* Add back support for Exemplars. See
[exemplars](../../docs/metrics/customizing-the-sdk/README.md#exemplars) for
instructions to enable exemplars.
([#4553](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4553))

* Updated to support `Severity` and `SeverityText` when exporting
`LogRecord`s.
([#4568](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4568))
* **[Experimental feature](./README.md#experimental-features)**
Add back support for Exemplars which is now treated as an experimental
feature available only in pre-release builds. See
[exemplars](../../docs/metrics/customizing-the-sdk/README.md#exemplars) for
instructions to enable exemplars.
([#4553](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4553))

* **[Experimental feature](./README.md#experimental-features)**
The `Severity` and `SeverityText` fields on `LogRecord` are only available in
pre-release builds of the SDK. Pre-release builds of the OTLP exporter will
export these fields.
([#4568](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4568))

## 1.5.1

Expand Down
8 changes: 6 additions & 2 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@

using System.Runtime.CompilerServices;
using Google.Protobuf;
#if EXPOSE_EXPERIMENTAL_FEATURES
using OpenTelemetry.Internal;
#endif
using OpenTelemetry.Logs;
#if EXPOSE_EXPERIMENTAL_FEATURES
using OpenTelemetry.Trace;
#endif
using OtlpCollector = OpenTelemetry.Proto.Collector.Logs.V1;
using OtlpCommon = OpenTelemetry.Proto.Common.V1;
using OtlpLogs = OpenTelemetry.Proto.Logs.V1;
Expand Down Expand Up @@ -79,7 +83,7 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord, SdkLimitO

var attributeValueLengthLimit = sdkLimitOptions.AttributeValueLengthLimit;
var attributeCountLimit = sdkLimitOptions.AttributeCountLimit ?? int.MaxValue;

#if EXPOSE_EXPERIMENTAL_FEATURES
utpilla marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Are we still going to rename the EventId keys? I think we should.

Copy link
Member

Choose a reason for hiding this comment

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

PS: Probably worth mentioning this in CHANGELOG.

Copy link
Member Author

@alanwest alanwest Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are we still going to rename the EventId keys? I think we should.

See conversation here #4762 (comment)

PS: Probably worth mentioning this in CHANGELOG.

Yes, please hold on merging this. I haven't submitted a changelog entry yet because I'm working on a method to document our experimental features somewhat centrally.

Copy link
Member Author

@alanwest alanwest Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@CodeBlanch @utpilla @vishweshbankwar @Kielek Take another look at this PR.

Besides updating the changelog, I reorganized it a bit to propose a slightly different pattern documenting experimental features not only in the changelog but also in the README. This way the documentation of our experimental features doesn't get buried in old changelog entries.

If this pattern looks good, I'm happy to follow up in a separate PR fixing up the documentation for the other components we have introduced experimental features for.

// First add the generic attributes like Category, EventId and Exception,
// so they are less likely being dropped because of AttributeCountLimit.

Expand Down Expand Up @@ -109,7 +113,7 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord, SdkLimitO
otlpLogRecord.AddStringAttribute(SemanticConventions.AttributeExceptionMessage, logRecord.Exception.Message, attributeValueLengthLimit, attributeCountLimit);
otlpLogRecord.AddStringAttribute(SemanticConventions.AttributeExceptionStacktrace, logRecord.Exception.ToInvariantString(), attributeValueLengthLimit, attributeCountLimit);
}

#endif
bool bodyPopulatedFromFormattedMessage = false;
if (logRecord.FormattedMessage != null)
{
Expand Down
25 changes: 25 additions & 0 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,31 @@ services.AddHttpClient(
Note: The single instance returned by `HttpClientFactory` is reused by all
export requests.

## Experimental features

The OTLP exporter contains the following
[experimental features](../../README.md#experimental-features):

* The
[exemplar specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplar)
is not yet stable. Pre-release builds of the OpenTelemetry .NET SDK and the
OTLP exporter support sampling and exporting exemplars.
* Support for exporting the following fields on `LogRecord`: `CategoryName`,
`EventId`, and `Exception`. These fields are unique to OpenTelemetry .NET's
data model (i.e., they are not reflected in the
[standard data model](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#log-and-event-record-definition)).
Therefore, the conventions
for exporting these fields is not yet stable. In pre-release builds, these
fields are exported as attributes mapped as follows:
* `CategoryName` maps to `dotnet.ilogger.category_name`.
* `EventId` maps to two attributes `Id` and `Name` representing
`Event.Id` and `EventId.Name`, respectively.
* `Exception` maps to the attributes defined by the experimental
[exception semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-logs.md#attributes).
* In pre-release builds of the OpenTelemetry .NET SDK `LogRecord` contains new
fields: `Severity` and `SeverityText`, and they will be exported by the OTLP
exporter.

## Troubleshooting

This component uses an
Expand Down
Loading