Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4297 +/- ##
==========================================
- Coverage 97.56% 97.49% -0.07%
==========================================
Files 529 532 +3
Lines 63765 64095 +330
Branches 1 1
==========================================
+ Hits 62212 62490 +278
- Misses 1553 1605 +52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
| if (links) { | ||
| this.links = links.map(truncateLink) | ||
| } |
There was a problem hiding this comment.
the otel implementation calls this.#processor.onStart(span, ctx), we do this in the tracer after the span is created, we should prob move it here ot be consistent
| } | ||
|
|
||
| isRecording() { | ||
| return !this.#ended |
There was a problem hiding this comment.
| return !this.#ended | |
| return this.#ended === false |
| #sampler | ||
| #processor | ||
|
|
||
| constructor({ instrumentationScope, sampler, processor }) { |
There was a problem hiding this comment.
this is prob fine but the otel Tracer allows options to be passed in. i'm trying to see if we need to consider them since this isn't controlled by user
There was a problem hiding this comment.
I'm not sure about that. The options upstream accepts are quite minimal https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_api._opentelemetry_api.TracerOptions.html and seem irrelevant to us (I don't know. Their docs don't actually explain what anything is or does.).
jsumners-nr
left a comment
jsumners-nr
left a comment
There was a problem hiding this comment.
I'd like to see at least one of the self-review comments addressed, but nothing in either review is a blocker for me. Everything seems reasonable.
| forceFlush() { | ||
| return Promise.resolve() | ||
| } | ||
|
|
||
| shutdown() { | ||
| return Promise.resolve() | ||
| } |
There was a problem hiding this comment.
These are methods on the baseline implementation upstream, not methods required by the interface. So they should be specific to the way upstream implements the other providers upstream supplies that extend from it.
Having stubs that match the outcome of upstream's implementations seems fine.
| #sampler | ||
| #processor | ||
|
|
||
| constructor({ instrumentationScope, sampler, processor }) { |
There was a problem hiding this comment.
I'm not sure about that. The options upstream accepts are quite minimal https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_api._opentelemetry_api.TracerOptions.html and seem irrelevant to us (I don't know. Their docs don't actually explain what anything is or does.).
There was a problem hiding this comment.
I won't block on this, but I do want to point out that I find these sorts of files more frustrating than helpful. The name of the file doesn't provide any hint as to what it contains or exports, and they tend to get filled with random stuff (even though this one is likely to remain small). Personally, I'd prefer to see:
lib/otel/traces/helpers/make-processor.jslib/otel/traces/helpers/make-sampler.js
This branch has not been deployed
3 participants
Description
This PR removes
@opentelemetry/sdk-trace-basein lieu of building our own Span, TracerProvider and Tracer. I want to look into the options based into the old BasicTracerProvider to ensure we aren't missing anything. The span attribute limit is directly enforced on the span, but unsure of any hidden items.Related Issues
Closes #4296