sdk/log: Drop duplicated KeyValues

[pellared]

Why

From
https://opentelemetry.io/docs/specs/otel/logs/data-model/#type-mapstring-any:

The keys in the map are unique (duplicate keys are not allowed).

Also: open-telemetry/opentelemetry-specification#3931 (comment)

Adding an option to avoid deduplication (and allow duplicated keys) is tracked as a separate issue. #5133

What

Drop duplicated KeyValues in simple and batch processor OR record.

More:

• sdk/log: Drop duplicated KeyValues #5086 (comment)
• sdk/log: Drop duplicated KeyValues #5086 (comment)
• Prototype (for logger, not processor): Drop keyvals with duplicated keys pellared/opentelemetry-go#10

[pellared]

There is a similar issue that indicates that users may want to have an option to avoid deduplication: #5130.

I created #5133

[pellared]

The problem of dropping duplicates in the logger is that they can be passed not only by the Bridge API but also by the processor (which can add/set attributes and set a body). Therefore, the duplicates should be deduplicated as far int the pipeline as possible.

Therefore, my proposal is that the OTLP exporters handle deduplication. We can have an option in the OTLP exporters to disable the deduplication to improve the OTLP exporter's performance.

This is looks to be acceptable:

• https://github.com/open-telemetry/opentelemetry-specification/pull/3938/files#r1526685159
• https://github.com/open-telemetry/opentelemetry-specification/pull/3938/files#r1525101870

Especially, by @tigrannajaryan:

I think it is fine to do the deduplication anywhere you want as long as externally observable data complies with this document.

I will try to make a second try and update the specification here to make it more clear. I plan to add something more or less like:

The SDKs MAY allow duplicates (represent this type as a list of key-values) as long as the OTLP exporters by default handle deduplication.

@open-telemetry/go-approvers Please leave 👍 if you agree, if so I will close the issue and update the description of:

• Add otlploghttp exporter #5057
• Add otlploggrpc exporter #5056

[pellared]

The alternative, could be creating a processor which would handle deduplication.

I am leaning to this approach as I find that it would be more-performant by default. It follows the behavior of all popular Go logging libraries. At last, even if we would do it by default - the user would be able to by-pass and duplicates at some point (e.g. by implementing a custom processor).

Please leave 👍 if you agree, if so if so I will close this issue and create a new one to add DedupProcessor.

I propose open-telemetry/opentelemetry-specification#3987 to allow flexibility.

[pellared]

Another alternative, could be adding deduplication to the simple and batch processor.

It is the best place to add if we would like to "enable deduplication be default". I assume most people would use the batch processor in production. We could still implement it via a DedupDecorator which would be applied by default. We can add options to the processors to not have them decorated.

Side note: Simple processor does not currently accept any options. I am not sure if we should add options or just focus on batch processor which is intended for production purposes.

Please leave 👍 if you agree, if so if so I will close this issue and create a new one to add DedupProcessor with a description that it should be used by default when creating a simple and batch processor.

[dashpole]

I think it is fine to do the deduplication anywhere you want as long as externally observable data complies with this document.

This makes it sound like the data passed to an exporter must already be deduplicated.

I think it is fair to provide non-default options like this, with a clear warning that the caller is responsible for ensuring there are no duplicates, assuming there is a need for such high performance.

Is tigran saying this can't be the default?

[pellared]

Is tigran saying this can't be the default

I am not sure if it is a suggestion or a requirement. Personally, I would start with no deduplication by default. We could introduce this behavior as default to the batch processor later (#5086 (comment)) if needed.

I have not heard about many problems because of the lack of deduplication in .NET. They also prefer it as opt-in. Reference: open-telemetry/opentelemetry-dotnet#4324

[MrAlias]

The alternative, could be creating a processor which would handle deduplication.

I am leaning to this approach as I find that it would be more-performant by default. It follows the behavior of all popular Go logging libraries. At last, even if we would do it by default - the user would be able to by-pass and duplicates at some point (e.g. by implementing a custom processor).

Please leave 👍 if you agree, if so if so I will close this issue and create a new one to add DedupProcessor.

I propose open-telemetry/opentelemetry-specification#3987 to allow flexibility.

I think we could add this afterwards. Given the OTLP seems to be the only place that de-duplication needs to be done we could start there and if a general processor is needed we could add that later.

[pellared]

if a general processor is needed we could add that later.

I think we should add options (even though there is none) to SimpleProcessor so that we can and a WithoutDedup option later if needed. If we don't do it now, then later it would be a breaking change. WDYT?

PS. My guts tell me that we would have to go with #5086 (comment).

[pellared]

Based on open-telemetry/opentelemetry-specification#3931 (comment) it looks like it it looks my guts (previous comment) were right.

[MrAlias]

Partial implementation of attribute de-duplication: #5190

[MrAlias]

Resolved by #5230