Skip to content

Remove 10 parameter limit in OpenTelemetry export#2221

Closed
mmarczell-graphisoft wants to merge 3 commits intoembrace-io:mainfrom
mmarczell-graphisoft:no_limit_on_otel_attrs
Closed

Remove 10 parameter limit in OpenTelemetry export#2221
mmarczell-graphisoft wants to merge 3 commits intoembrace-io:mainfrom
mmarczell-graphisoft:no_limit_on_otel_attrs

Conversation

@mmarczell-graphisoft
Copy link
Copy Markdown

Goal

The Android SDK caps the number of custom properties in a log event to 10, even when external OpenTelemetry export is used. The iOS SDK doesn't. This PR lets the OpenTelemetry export contain all the properties without limitation, while the logs sent to the Embrace backend are capped to 10 properties as before.

Testing

  • PropertiesTest now verifies that PropertyUtils does not cap the property count
  • EmbraceLogRecordDataTest now verifies that toEmbracePayload does cap the property count

Release Notes

The number of log properties in the OpenTelemetry export is no longer capped to 10.

@mmarczell-graphisoft mmarczell-graphisoft requested a review from a team as a code owner May 29, 2025 09:58
Copy link
Copy Markdown
Contributor

This looks good! I'm going to check a couple things and then merge it after you make the changes to get it to pass CI (it seems to be a detekt failure so running ./gradlew detekt should fix them for you automatically.

There are a couple of additional changes we'll want to make on top of this but it gives us a good basis from which to work. Thanks for contributing!


fun Attributes.toEmbracePayload(): List<Attribute> =
this.asMap().entries.map { Attribute(it.key.key, it.value.toString()) }
this.asMap().entries
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@priettt pointed out that you should reverse the order of take and map - feel free to fix it along with the detekt error or we can deal with it when we make the additional changes (we may change where the truncation happens so don't spend too much time on it).

@bidetofevil
Copy link
Copy Markdown
Contributor

Hey @mmarczell-graphisoft - looks like there are still some test failures because we were expecting the same data in the export as in the internal representation.

I did some refactoring of our internals and reimplemented what you did in #2225, which should take care of your use case.

Can you test it when it merges?

@mmarczell-graphisoft
Copy link
Copy Markdown
Author

@bidetofevil I can test it when it merges, no problem.

@mmarczell-graphisoft
Copy link
Copy Markdown
Author

@bidetofevil I tested main and it worked, thanks

@bidetofevil
Copy link
Copy Markdown
Contributor

Excellent! Do you still need any part of the PR or can it be closed?

If main works as is, we will be doing a release later this week or the next so you'll be able to consume the change in an official release.

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.

2 participants