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

Support OTEL_SPAN_ATTRIBUTE_SIZE_LIMIT #1847

Closed
wants to merge 1 commit into from

Conversation

owais
Copy link
Contributor

@owais owais commented May 12, 2021

Description

Adds support for OTEL_SPAN_ATTRIBUTE_SIZE_LIMIT to allow limiting/truncating string values in attributes.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Updated and added tests

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@owais owais force-pushed the max-attr-length-limit branch from 48264f2 to 1c1cbab Compare May 12, 2021 15:57
@owais
Copy link
Contributor Author

owais commented May 12, 2021

This isn't ready yet as it is not clear what the final iteration of the proposal will look like but feel free to review and provide feedback here on on the proposal.

@lzchen
Copy link
Contributor

lzchen commented May 14, 2021

Please assign yourself to #1845 if you are working on this.

@owais owais force-pushed the max-attr-length-limit branch 4 times, most recently from 00f90fd to 92b72fe Compare May 15, 2021 01:24
@owais owais force-pushed the max-attr-length-limit branch 17 times, most recently from 95a78b0 to 19b675b Compare May 26, 2021 18:31
@owais owais marked this pull request as ready for review May 26, 2021 18:56
@owais owais requested review from a team, codeboten and ocelotl and removed request for a team May 26, 2021 18:56
@owais owais added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label May 26, 2021
@owais owais removed the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label May 26, 2021
@owais owais changed the title Max attr length limit Support OTEL_SPAN_ATTRIBUTE_SIZE_LIMIT May 27, 2021
CHANGELOG.md Outdated
@@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#1829](https://github.com/open-telemetry/opentelemetry-python/pull/1829))
- Lazily read/configure limits and allow limits to be unset.
([#1839](https://github.com/open-telemetry/opentelemetry-python/pull/1839))
- Added support for read/configure limits and allow limits to be unset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this description accurate?

@lzchen
Copy link
Contributor

lzchen commented May 27, 2021

Could you add a link to where this is defined in the specs?

_clean_attributes(
resource_attributes, self._limits.max_attribute_size
)
self._resource = Resource(resource_attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we apply _clean_attributes in the Resource constructor instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Limits and Resource instances both are provided to TracerProvider as arguments. If we want to do this inside Resource, we need to be able to pass Limits to Resource constructor and expect users to do it. Another thing we can do is add a method on Resource and call it from here so it'd look like:

self._resource = self._resource._with_limits(self._limits)

This will allow us to call _clean_attributes from within Resource class but not sure how much better that would be as the end result would be pretty much the same i.e, new instance of resource will be created given it is immutable.

Yet another thing we can do is to not apply these limits to resource attributes for now. In practice these limits are useful for spans where an instrumentation might generate very large fields like storing very long stack traces as attributes. Such issues don't plague resources so I'm completely fine with not applying these limits to resource attributes.

@owais owais force-pushed the max-attr-length-limit branch from 19b675b to 0ed5768 Compare June 1, 2021 23:19
@owais owais force-pushed the max-attr-length-limit branch from 0ed5768 to 5be7a88 Compare June 1, 2021 23:49
@owais owais marked this pull request as draft June 1, 2021 23:55
@owais
Copy link
Contributor Author

owais commented Aug 17, 2021

Closing in favor of #2044

@owais owais closed this Aug 17, 2021
@owais owais deleted the max-attr-length-limit branch August 17, 2021 09:53
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