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

feat(opentelemetry-sdk-trace-base): implemented general limits of attributes #2430

Merged

Conversation

banothurameshnaik
Copy link
Contributor

@banothurameshnaik banothurameshnaik commented Aug 26, 2021

Which problem is this PR solving?

Short description of the changes

  • Implemented OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT and OTEL_ATTRIBUTE_COUNT_LIMIT attributes as general tracer attributes.
  • Modified span attribute limits accordingly to general attribute limits
  • When only general attribute limits defined, span attribute limits will be considered as general limits
  • When both general, span-specific limits are defined, span-specific limits will be considered as priority

@banothurameshnaik banothurameshnaik requested a review from a team August 26, 2021 17:09
@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #2430 (496ab0a) into main (3acebdc) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2430      +/-   ##
==========================================
- Coverage   93.22%   93.21%   -0.01%     
==========================================
  Files         137      137              
  Lines        5002     5014      +12     
  Branches     1057     1060       +3     
==========================================
+ Hits         4663     4674      +11     
- Misses        339      340       +1     
Impacted Files Coverage Δ
...ackages/opentelemetry-sdk-trace-base/src/config.ts 86.84% <ø> (ø)
...ckages/opentelemetry-core/src/utils/environment.ts 96.00% <100.00%> (+0.16%) ⬆️
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts 98.46% <100.00%> (+0.07%) ⬆️
...ckages/opentelemetry-sdk-trace-base/src/utility.ts 100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@banothurameshnaik
Copy link
Contributor Author

Can someone review this PR?

@rauno56

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Well done. Sorry for taking so long to review!

@rauno56
Copy link
Member

rauno56 commented Sep 6, 2021

It looks like merging broke the this PR. Please take a look.

@banothurameshnaik
Copy link
Contributor Author

I will check broken changes, thanks

@banothurameshnaik
Copy link
Contributor Author

@rauno56 I fixed the broken changes.

@dyladan dyladan added the enhancement New feature or request label Sep 8, 2021
@@ -212,6 +214,11 @@ export class Tracer implements api.Tracer {
return api.context.with(contextWithSpanSet, fn, undefined, span);
}

/** Returns the active {@link GeneralLimits}. */
getGeneralLimits(): GeneralLimits {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have this function? It is not part of the interface. If it is just for testing, you can access the private field by doing tracer["_generalLimits"] in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can access the way you mentioned.

I just followed the pattern we already have like getSpanLimits(): SpanLimits { in line 223

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

Successfully merging this pull request may close these issues.

Spec: implement an option to limit length of values of attributes and metric values
4 participants