-
Notifications
You must be signed in to change notification settings - Fork 529
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
chore(deps): update deps matching "@opentelemetry/" #2408
chore(deps): update deps matching "@opentelemetry/" #2408
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2408 +/- ##
==========================================
- Coverage 90.97% 90.64% -0.34%
==========================================
Files 146 156 +10
Lines 7492 7612 +120
Branches 1502 1571 +69
==========================================
+ Hits 6816 6900 +84
- Misses 676 712 +36
|
@@ -96,7 +96,7 @@ export class BunyanInstrumentation extends InstrumentationBase<BunyanInstrumenta | |||
} | |||
|
|||
override setConfig(config: BunyanInstrumentationConfig = {}) { | |||
this._config = { ...DEFAULT_CONFIG, ...config }; |
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.
reviewer note: this did override the enabled
config and actually disabled the instrumentation, this went unnoticed until the change from open-telemetry/opentelemetry-js@0ee398d which now uses this method instead of setting it directly.
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.
you mention on the description the update on @opentelemetry/semantic-conventions
, but I don't see the updates on this dependency
I think we exclusively caret depend on |
…correct span kind
…n, use correct span kind
export enum OtlpSpanKind { | ||
UNSPECIFIED = 0, | ||
INTERNAL = 1, | ||
SERVER = 2, | ||
CLIENT = 3, | ||
PRODUCER = 4, | ||
CONSUMER = 5, | ||
} | ||
|
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.
reviewer note: some tests incorrectly used SpanKind
instead of the OTLP representation of the same enum. The number values are different, but the tests that used it did look somewhat plausible.
INTERNAL spans were asserted to be SERVER spans, the first SERVER span was asserted as a CLIENT span as they shared the same value.
I opted to add this here as we'll change the exports from @opentelemetry/otlp-transformer
soon-ish and that type we have there will not be exported anymore. Not using the type from there will help save use some work when we have to clean up the tests in this repo eventually.
assert.strictEqual(spans[0].name, 'GET'); | ||
assert.strictEqual(spans[0].kind, testUtils.OtlpSpanKind.CLIENT); |
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.
reviewer note: with open-telemetry/opentelemetry-js#4866 http/https client instrumentation was fixed and now produces a client span.
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.
Oh wow, this makes so much more sense. Thanks for fixing this!
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.
Thanks for chasing this down, Marc!
Sorry for the confusion.
FWIW, the testUtils.runTestFixture()
stuff was based on a similar thing we have at work on a separate repo. In that case the equivalent of the TestCollector class normalizes the span objects somewhat. One of those normalizations is to translate the OTLP span.kind
and span.status
values to their enum strings. So, test asserts are doing assertEqual(span.kind, 'SPAN_KIND_CLIENT')
and assertEqual(span.status.code, 'STATUS_CODE_ERROR')
.
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.
Again, FWIW, here is the normalization code I mean: https://github.com/elastic/elastic-otel-node/blob/main/packages/mockotlpserver/lib/normalize.js#L114-L154
I'm not proposing we use this in this repo.
I'd vaguely recall seeing something about a canonical JSON representation of OTLP service requests or of OTel signals. Given that, I think that would be a good target for normalization, and then tests could assert against that normalized form.
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.
#2421 to get one more occurrence of SpanKind.*
usage in this test file.
assert.strictEqual(spans[0].name, 'GET'); | ||
assert.strictEqual(spans[0].kind, testUtils.OtlpSpanKind.CLIENT); | ||
assert.strictEqual(spans[1].name, 'GET /post/:id'); | ||
assert.strictEqual(spans[1].kind, testUtils.OtlpSpanKind.SERVER); |
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.
reviewer note: this was previously asserted to be a client span, which it actually was not. Since the SDK-internal representation for a client span is 2
(SpanKind.CLIENT
) and the OTLP represenation for a server span is 2
(OtlpSpanKind.SERVER
), the test passed anyway.
It now asserts on the correct kind.
assert.strictEqual(spans[2].kind, testUtils.OtlpSpanKind.INTERNAL); | ||
assert.strictEqual(spans[2].parentSpanId, spans[1].spanId); | ||
assert.strictEqual(spans[3].name, 'middleware - expressInit'); | ||
assert.strictEqual(spans[3].kind, testUtils.OtlpSpanKind.INTERNAL); |
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.
reviewer note: the same thing I described above also happened with SpanKind.SERVER
and OtlpSpanKind.INTERNAL
. I also adapted all following tests.
assert.strictEqual(spans[0].name, 'GET'); | ||
assert.strictEqual(spans[0].kind, testUtils.OtlpSpanKind.CLIENT); | ||
assert.strictEqual(spans[1].name, 'GET /post/:id'); | ||
assert.strictEqual(spans[1].kind, testUtils.OtlpSpanKind.SERVER); | ||
assert.strictEqual(spans[2].name, 'middleware - simpleMiddleware'); | ||
assert.strictEqual(spans[2].kind, testUtils.OtlpSpanKind.INTERNAL); | ||
assert.strictEqual(spans[2].parentSpanId, spans[1].spanId); | ||
assert.strictEqual(spans[3].name, 'router - /post/:id'); | ||
assert.strictEqual(spans[3].kind, testUtils.OtlpSpanKind.INTERNAL); | ||
assert.strictEqual(spans[3].parentSpanId, spans[2].spanId); |
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.
reviewer note: the same thing as in the express instrumentation is happening here as well, SpanKind SDK vs. OTLP-representation and an additional span that is inserted at positon 0.
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 is great, thank you for fixing up the confusion with span kind and sorry for causing some of that headache with the tests! @robbkidd and I were just looking at that earlier today thinking something was off there.... This makes much more sense now.
assert.strictEqual(spans[0].name, 'GET'); | ||
assert.strictEqual(spans[0].kind, testUtils.OtlpSpanKind.CLIENT); |
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.
Oh wow, this makes so much more sense. Thanks for fixing this!
No worries. 🙂 I also remember looking at them at some point but I did not think too much of it - they looked plausible enough for me not to second guess it before they were failing. 😬 Fortunately it was an easy fix 🙂 |
…also fix test flakiness This is a follow-up on open-telemetry#2408 (comment) to update one more usage of `SpanKind.*` to `testUtils.OtlpSpanKind.*`. While testing this locally I also noticed that the instr-express tests using runTestFixture are flaky. The issue is that `TestCollector#sortedSpans` can get the span ordering wrong when spans start in the same millisecond (the resolution of span.startTimeUnixNano). This happens with Express middleware spans on a fast machine. I've updated `sortedSpans` to fallback to using span.parentSpanId to attempt to get more reliable sorting.
…also fix test flakiness (#2421) This is a follow-up on #2408 (comment) to update one more usage of SpanKind.* to testUtils.OtlpSpanKind.*. While testing this locally I also noticed that the instr-express tests using runTestFixture are flaky. The issue is that TestCollector#sortedSpans can get the span ordering wrong when spans start in the same millisecond (the resolution of span.startTimeUnixNano). This happens with Express middleware spans on a fast machine. I've updated sortedSpans to fallback to using span.parentSpanId to attempt to get more reliable sorting.
BEGIN_COMMIT_OVERRIDE
feat: update deps matching "@opentelemetry/"
END_COMMIT_OVERRIDE