OTLP LogExporter to enable ParseStateValues by default

[cijothomas]

See #3183 (comment)

[codecov]

Codecov Report

Merging #3186 (14e7fe0) into main (50baa25) will increase coverage by 0.57%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3186      +/-   ##
==========================================
+ Coverage   85.17%   85.75%   +0.57%     
==========================================
  Files         259      260       +1     
  Lines        9345     9358      +13     
==========================================
+ Hits         7960     8025      +65     
+ Misses       1385     1333      -52     

Impacted Files	Coverage Δ
...ryProtocol.Logs/OtlpLogExporterHelperExtensions.cs	92.30% <100.00%> (ø)
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs	94.11% <0.00%> (+11.76%)	⬆️
...etryProtocol/Implementation/LogRecordExtensions.cs	94.52% <0.00%> (+16.43%)	⬆️
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs	70.00% <0.00%> (+70.00%)	⬆️
...ementation/ExportClient/OtlpGrpcLogExportClient.cs	78.57% <0.00%> (+78.57%)	⬆️
...penTelemetry/Logs/BatchLogRecordExportProcessor.cs	100.00% <0.00%> (+100.00%)	⬆️

[reyang]

LGTM.

[cijothomas]

@alanwest @CodeBlanch Please share your thoughts/comments. I think this is a reasonable way to proceed. (will do similar change for ConsoleExporter as well), by letting individual Exporters to make it easy for their users to configure ParseStateValues. If an Exporter wants to still do its own parsing of "State", its totally legal, as State was part of LogRecord, before StateValues.

[CodeBlanch]

Might be useful to have a comment here explaining why it is being done?

[CodeBlanch]

Feels a bit clunky but I don't have a better solution 😄

Should we add more to the comments here calling out that exporters may automatically enable ParseStateValues if they support the feature?

opentelemetry-dotnet/src/OpenTelemetry/Logs/OpenTelemetryLoggerOptions.cs

Lines 42 to 51 in 50baa25

	/// <summary>
	/// Gets or sets a value indicating whether or not log state should be
	/// parsed into <see cref="LogRecord.StateValues"/> on generated <see
	/// cref="LogRecord"/>s. Default value: False.
	/// </summary>
	/// <remarks>
	/// Note: When <see cref="ParseStateValues"/> is set to <see
	/// langword="true"/> <see cref="LogRecord.State"/> will always be <see
	/// langword="null"/>.
	/// </remarks>

[alanwest]

I agree feels a bit clunky for AddOltpExporter to have the side effect of changing an option - though I also agree that ParseStateValues=true seems like the most reasonable default.

Why not the proposal from this comment? Are we considering it inefficient because prevents state from being garbage collected? Or some other reason? #3183 (comment).

If we consider the default (ParseStateValues = false) as a bad default, we could change the default to make it true. To avoid breaking existing users, the SDK can continue to populate State as well. That way, exporters need to only deal with the parse state values, and if any exporters were previously relying on the State, they will be unaffected as well...

---

Another alternative may be to make the setting bool? ParseStateValues. Default is null - i.e., unspecified. Makes it clear that the actual default depends on the exporter you've configured. This is similar to what I did with PeriodicExportingMetricReaderOptions.

opentelemetry-dotnet/src/OpenTelemetry/Metrics/PeriodicExportingMetricReaderOptions.cs

Lines 19 to 26 in a709cfd

	public class PeriodicExportingMetricReaderOptions
	{
	/// <summary>
	/// Gets or sets the metric export interval in milliseconds.
	/// If not set, the default value depends on the type of metric exporter
	/// associated with the metric reader.
	/// </summary>
	public int? ExportIntervalMilliseconds { get; set; }

I guess this would be a breaking change for anyone who's written an extension that uses the LoggingOptions class... maybe ok?

[alanwest]

Approving, but curious of folks thoughts on questions I posed.

[CodeBlanch]

@alanwest

If we consider the default (ParseStateValues = false) as a bad default, we could change the default to make it true. To avoid breaking existing users, the SDK can continue to populate State as well. That way, exporters need to only deal with the parse state values, and if any exporters were previously relying on the State, they will be unaffected as well...

I liked this too initially. But then looking at some code, for example:

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/1fffa1e265019bf03a7d1361fe46c8c2fb88dbba/src/OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs#L201

If we switched the behavior so we always set state, anyone who has their ordering as "check state, then check StateValues" we might be breaking those exporters if we do that.

Another alternative may be to make the setting bool? ParseStateValues. Default is null - i.e., unspecified. Makes it clear that the actual default depends on the exporter you've configured.

I like that. Might still be breaking. We could [Obsolete] ParseStateValues and add bool DisableState { get; set; } maybe? If user sets DisableState = true we know they truly don't want anything. If DisableState = false we treat that as ParseStateValues = true today?

[alanwest]

I like that. Might still be breaking. We could [Obsolete] ParseStateValues and add bool DisableState { get; set; } maybe? If user sets DisableState = true we know they truly don't want anything. If DisableState = false we treat that as ParseStateValues = true today?

Yea, you're right it would definitely be a breaking change. It kinda seems like someone would need to be doing something kinda funky to be affected by the change, but obviously that's not a good enough reason for a breaking change 😄.

I'm not opposed to this idea of adding Obsolete. Feels a little dirty, but it's certainly something we've considered in other contexts - like the OTLP batch activity processor options. I think it might be a bit cleaner than an unexpected side affect of calling AddOtlpExporter.

[cijothomas]

Merging to continue fixing the rest of OTLP LogExporter issues.
The question of how to deal with state definitely is worth coming back to, once we have a fully working solution for end users.