-
Notifications
You must be signed in to change notification settings - Fork 805
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
feat(sdk-trace-base): add dropped attributes and events count on span #3576
Conversation
|
…ub.com:mohitk05/opentelemetry-js into feat/span-dropped-counts
There was a problem hiding this 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 Report
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
|
…d zipkin exporters
…ub.com:mohitk05/opentelemetry-js into feat/span-dropped-counts
I am using the following approach now: Let me know if this is the ideal way, or if the counts should be added as attributes directly in the mutable |
There was a problem hiding this 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
experimental/packages/otlp-proto-exporter-base/tsconfig.esnext.json
Outdated
Show resolved
Hide resolved
Please add a changelog entry |
Dismissed my change request |
Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>
Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>
It's fine to do this in a followup PR. But I think we should document this somehow (maybe just an issue) that |
Sure, what would be the best way to do this? |
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? |
seems making droppedAttributedCount in TimedEvent results in problems so maybe best to make it optional again. Sorry. |
No worries, I updated the tests to follow the type. |
@Flarna I have made |
I think optional is fine since it comes from the underlying proto definition |
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) |
There was a problem hiding this comment.
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.
* 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 I've resolved your comments. |
Which problem is this PR solving?
Adds two new properties on
ReadableSpan
:droppedAttributesCount
: A number storing the count of dropped attributes of a spandroppedEventsCount
: A number storing the count of dropped events of a spandroppedLinksCount
: 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
andaddEvent
methods onSpan
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.
Checklist: