OTLP Exporter: Conditionally compile experimental log features #4762

alanwest · 2023-08-11T00:52:46Z

I want to prioritize shipping a stable OTLP log exporter. While there are important conversations to be had to fully resolve this conversation #4754 (comment), it should not block a stable exporter release.

For our upcoming stable release, this PR will exclude the following fields on LogRecord from being exported by the OTLP exporter:

EventId
CategoryName
Exception

The first two fields are not part of the OpenTelemetry log data model, so exporting them requires us to carefully consider the conventions we use to map them.

There are experimental conventions defined for Exception, however my preference would be to exclude all three of these fields for now and later expose them all via feature flag.

codecov · 2023-08-11T01:42:09Z

Codecov Report

Merging #4762 (10374f4) into main (3b4db6d) will increase coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 93.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4762      +/-   ##
==========================================
+ Coverage   82.09%   82.13%   +0.03%     
==========================================
  Files         313      313              
  Lines       12784    12783       -1     
==========================================
+ Hits        10495    10499       +4     
+ Misses       2289     2284       -5

Files Changed	Coverage Δ
...etryProtocol/Implementation/LogRecordExtensions.cs	`92.50% <ø> (ø)`
...DiagnosticSourceInstrumentation/PropertyFetcher.cs	`97.43% <93.75%> (+12.43%)`	⬆️

…strumentation-is-experimental

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs

CodeBlanch · 2023-08-11T16:44:42Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs

@@ -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


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

PS: Probably worth mentioning this in CHANGELOG.

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.

@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.

CodeBlanch · 2023-08-11T19:39:25Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md

@@ -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)**


@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 😄

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

Experimental features offered via public APIs.

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 :)

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 😄.

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.

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

vishweshbankwar · 2023-08-11T20:40:44Z

README.md

@@ -104,6 +104,23 @@ extension scenarios:
 * Building a custom sampler for
  [traces](./docs/trace/extending-the-sdk/README.md#sampler).

+## Experimental Features
+


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.

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.

alanwest · 2023-08-18T20:11:32Z

We've settled on #4781

Conditionally compile experimental log features

5c3188c

alanwest requested a review from a team August 11, 2023 00:52

alanwest mentioned this pull request Aug 11, 2023

[Otlp] Rename ilogger event id and name attributes #4754

Closed

4 tasks

alanwest added 2 commits August 10, 2023 18:15

Stylecops

96a280b

Stylecops

0a5669d

alanwest added 2 commits August 10, 2023 18:48

Stylecops

2dea303

Merge remote-tracking branch 'upstream/main' into alanwest/ilogger-in…

5b00c21

…strumentation-is-experimental

utpilla reviewed Aug 11, 2023

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs Show resolved Hide resolved

utpilla approved these changes Aug 11, 2023

View reviewed changes

vishweshbankwar approved these changes Aug 11, 2023

View reviewed changes

Kielek approved these changes Aug 11, 2023

View reviewed changes

CodeBlanch reviewed Aug 11, 2023

View reviewed changes

alanwest added 2 commits August 11, 2023 11:21

Document experimental features

6dfcefb

Update readme

10374f4

CodeBlanch reviewed Aug 11, 2023

View reviewed changes

vishweshbankwar reviewed Aug 11, 2023

View reviewed changes

alanwest mentioned this pull request Aug 18, 2023

[otlp] Remove ilogger and exception attributes #4781

Merged

4 tasks

alanwest closed this Aug 18, 2023

alanwest deleted the alanwest/ilogger-instrumentation-is-experimental branch August 18, 2023 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTLP Exporter: Conditionally compile experimental log features #4762

OTLP Exporter: Conditionally compile experimental log features #4762

alanwest commented Aug 11, 2023 •

edited

Loading

codecov bot commented Aug 11, 2023 •

edited

Loading

CodeBlanch Aug 11, 2023

CodeBlanch Aug 11, 2023

alanwest Aug 11, 2023 •

edited

Loading

alanwest Aug 11, 2023 •

edited

Loading

CodeBlanch Aug 11, 2023

vishweshbankwar Aug 11, 2023

alanwest Aug 11, 2023

CodeBlanch Aug 11, 2023

vishweshbankwar Aug 11, 2023

vishweshbankwar Aug 11, 2023 •

edited

Loading

utpilla Aug 11, 2023 •

edited

Loading

alanwest commented Aug 18, 2023

OTLP Exporter: Conditionally compile experimental log features #4762

OTLP Exporter: Conditionally compile experimental log features #4762

Conversation

alanwest commented Aug 11, 2023 • edited Loading

codecov bot commented Aug 11, 2023 • edited Loading

Codecov Report

CodeBlanch Aug 11, 2023

Choose a reason for hiding this comment

CodeBlanch Aug 11, 2023

Choose a reason for hiding this comment

alanwest Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

alanwest Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

CodeBlanch Aug 11, 2023

Choose a reason for hiding this comment

vishweshbankwar Aug 11, 2023

Choose a reason for hiding this comment

alanwest Aug 11, 2023

Choose a reason for hiding this comment

CodeBlanch Aug 11, 2023

Choose a reason for hiding this comment

vishweshbankwar Aug 11, 2023

Choose a reason for hiding this comment

vishweshbankwar Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

utpilla Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

alanwest commented Aug 18, 2023

alanwest commented Aug 11, 2023 •

edited

Loading

codecov bot commented Aug 11, 2023 •

edited

Loading

alanwest Aug 11, 2023 •

edited

Loading

alanwest Aug 11, 2023 •

edited

Loading

vishweshbankwar Aug 11, 2023 •

edited

Loading

utpilla Aug 11, 2023 •

edited

Loading