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(sdk-trace-base): add dropped attributes and events count on span #3576

Merged
merged 36 commits into from
Mar 8, 2023

Conversation

mohitk05
Copy link
Contributor

@mohitk05 mohitk05 commented Jan 27, 2023

Which problem is this PR solving?

Adds two new properties on ReadableSpan:

  • droppedAttributesCount: A number storing the count of dropped attributes of a span
  • droppedEventsCount: A number storing the count of dropped events of a span
  • droppedLinksCount: A number storing the count of dropped links of a span (currently no links are dropped, so this is always zero)

Fixes #2272

Short description of the changes

Adds two read-only objects in the Span class.

The PR updates the setAttribute and addEvent methods on Span to check for limits. When limits are reached, the counters are incremented at each occurrence of a dropped attribute/event.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Covered by unit tests.

  • Should have the right count of dropped attributes/events

Checklist:

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

@mohitk05 mohitk05 requested a review from a team January 27, 2023 23:24
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 27, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: mohitk05 / name: Mohit Karekar (b2c9f40)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

According to spec these should be reported as span attributes, not top level items.

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #3576 (474f8a9) into main (95bf6fc) will decrease coverage by 0.28%.
The diff coverage is 70.96%.

❗ Current head 474f8a9 differs from pull request most recent head 2221770. Consider uploading reports for the commit 2221770 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3576      +/-   ##
==========================================
- Coverage   93.63%   93.36%   -0.28%     
==========================================
  Files         275      257      -18     
  Lines        8095     7822     -273     
  Branches     1682     1646      -36     
==========================================
- Hits         7580     7303     -277     
- Misses        515      519       +4     
Impacted Files Coverage Δ
...ackages/opentelemetry-sdk-trace-base/src/config.ts 88.37% <ø> (ø)
...ges/opentelemetry-exporter-jaeger/src/transform.ts 93.33% <50.00%> (-6.67%) ⬇️
...ges/opentelemetry-exporter-zipkin/src/transform.ts 90.90% <57.14%> (-9.10%) ⬇️
packages/opentelemetry-sdk-trace-base/src/Span.ts 97.38% <83.33%> (-1.20%) ⬇️
...al/packages/otlp-transformer/src/trace/internal.ts 96.77% <100.00%> (+0.22%) ⬆️
...ckages/opentelemetry-core/src/utils/environment.ts 92.53% <100.00%> (+0.22%) ⬆️
api/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
... and 24 more

@mohitk05
Copy link
Contributor Author

mohitk05 commented Feb 5, 2023

According to spec these should be reported as span attributes, not top level items.

I am using the following approach now:
Expose the dropped counts on ReadableSpan so that the non-otlp exporters can use them as attributes. In each of jaeger and zipkin exporters I've added the counts as tags as per the specification. I believe the counts have to be exposed via ReadableSpan for them to be accessible in non-otlp exporters, and then the otel.* tags should be added only to the non-otel exporters, but for OTLP, they should still be span.*count.

Let me know if this is the ideal way, or if the counts should be added as attributes directly in the mutable Span.ts (i.e. span.attributes.droppedAttributesCount vs span.droppedAttributesCount).

@mohitk05 mohitk05 requested a review from dyladan February 7, 2023 09:29
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Requesting changes because of the modified tsconfig

@dyladan
Copy link
Member

dyladan commented Feb 24, 2023

Please add a changelog entry

@dyladan dyladan self-requested a review February 24, 2023 16:58
@dyladan
Copy link
Member

dyladan commented Feb 24, 2023

Dismissed my change request

mohitk05 and others added 2 commits March 1, 2023 14:37
Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>
Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>
@Flarna
Copy link
Member

Flarna commented Mar 1, 2023

It's fine to do this in a followup PR. But I think we should document this somehow (maybe just an issue) that OTEL_SPAN_ATTRIBUTE_PER_EVENT_COUNT_LIMIT and OTEL_SPAN_ATTRIBUTE_PER_LINK_COUNT_LIMIT have no effect yet.

@mohitk05
Copy link
Contributor Author

mohitk05 commented Mar 1, 2023

It's fine to do this in a followup PR. But I think we should document this somehow (maybe just an issue) that OTEL_SPAN_ATTRIBUTE_PER_EVENT_COUNT_LIMIT and OTEL_SPAN_ATTRIBUTE_PER_LINK_COUNT_LIMIT have no effect yet.

Sure, what would be the best way to do this?

@Flarna
Copy link
Member

Flarna commented Mar 1, 2023

The doc of all supported env vars would be the right place but there is no such document ;o),

I think a Github issue in this repo is enough. If you plan to work on this we can assign it to you otherwise others can pick it up.

@mohitk05
Copy link
Contributor Author

mohitk05 commented Mar 1, 2023

The doc of all supported env vars would be the right place but there is no such document ;o),

I think a Github issue in this repo is enough. If you plan to work on this we can assign it to you otherwise others can pick it up.

Okay, sure. I'll create an issue and can pick it up later, I do have the context so that'll help.

I have resolved the above comments, I see a couple of tests failing for me locally but they seemed unrelated. Can you trigger the workflow again?

@Flarna
Copy link
Member

Flarna commented Mar 1, 2023

seems making droppedAttributedCount in TimedEvent results in problems so maybe best to make it optional again. Sorry.

@mohitk05
Copy link
Contributor Author

mohitk05 commented Mar 1, 2023

No worries, I updated the tests to follow the type.

@mohitk05
Copy link
Contributor Author

mohitk05 commented Mar 2, 2023

@Flarna I have made droppedAttributesCount optional again in TimedEvent and Link. I recalled why I had done that - since the attributes property on both these interfaces is optional, it does not make sense to make droppedAttributesCount required.

@dyladan
Copy link
Member

dyladan commented Mar 3, 2023

I think optional is fine since it comes from the underlying proto definition

api/src/trace/link.ts Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -19,6 +19,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
* perf(propagator-jaeger): improve deserializeSpanContext performance [#3541](https://github.com/open-telemetry/opentelemetry-js/pull/3541) @doochik
* feat: support TraceState in SamplingResult [#3530](https://github.com/open-telemetry/opentelemetry-js/pull/3530) @raphael-theriault-swi
* feat(sdk-trace-base): add diagnostic logging when spans are dropped [#3610](https://github.com/open-telemetry/opentelemetry-js/pull/3610) @neoeinstein
* feat(sdk-trace-base): expose dropped counts for attributes, events and links on span [#3576](https://github.com/open-telemetry/opentelemetry-js/pull/3576)
Copy link
Member

Choose a reason for hiding this comment

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

This applies to more than just trace base. I think it's acceptable for the subject to apply to multiple packages.

Suggested change
* feat(sdk-trace-base): expose dropped counts for attributes, events and links on span [#3576](https://github.com/open-telemetry/opentelemetry-js/pull/3576)
* feat(tracing): expose dropped counts for attributes, events and links on span [#3576](https://github.com/open-telemetry/opentelemetry-js/pull/3576)

@dyladan dyladan added api:traces Issues and PRs related to the Traces API sdk:traces Issues and PRs related to the Traces SDK labels Mar 3, 2023
@mohitk05
Copy link
Contributor Author

mohitk05 commented Mar 7, 2023

@dyladan I've resolved your comments.

@legendecas legendecas merged commit 48ecf22 into open-telemetry:main Mar 8, 2023
@fuaiyi fuaiyi mentioned this pull request Mar 28, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api:traces Issues and PRs related to the Traces API sdk:traces Issues and PRs related to the Traces SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose counts for dropped attributes/events/links in ReadableSpan
4 participants