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.

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

Remove the duplicate key/value when ingesting log from opentelemetry-dotnet #4394

Conversation

hoangthanh28
Copy link

Description

This PR is intended to resolve the duplicate key exception while ingesting the otel-dotnet log.

Issues Resolved

Resolves #3868

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Collaborator

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Change looks good to me. I wonder, whether we want tests for that. But this is just Java Streams API. @hoangthanh28, check your commit messages for correct DCO: "signed-off-by: ..."

@dlvenable
Copy link
Member

@hoangthanh28 , Thank you for this contribution! I agree with @KarstenSchnitter that we should have a unit test. You can write a test which sends in multiple values and make sure you get the last one.

@hoangthanh28 hoangthanh28 force-pushed the features/thanh/otel_collector_key_duplication branch from 4b16bc6 to a583b93 Compare April 13, 2024 07:58
@hoangthanh28 hoangthanh28 force-pushed the features/thanh/otel_collector_key_duplication branch from a583b93 to 24ae806 Compare April 13, 2024 08:03
Copy link
Collaborator

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Thanks for providing the change and the unit tests. I am not sure, whether the test should enforce the decision for the first occurring key-value pair over potential later duplicates.

.addAllAttributes(Arrays.asList(duplicateAttribute1, duplicateAttribute2)).build();

Map<String, Object> map = OTelProtoCodec.convertKeysOfDataPointAttributes(actual);
assertThat(map.size(), is(equalTo(1)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand, that it is enough to test for non-duplication. However, this is unspecified behaviour, when the key values are different. It is not clear, which value will be chosen. Even if this is undocumented behaviour, I think the unit test should force a specific selection. This is how I understand @dlvenable comment: #4394 (comment)

@hoangthanh28
Copy link
Author

Merge from original repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Duplicate key exception when integrate with open-telemetry exporter
3 participants