-
Notifications
You must be signed in to change notification settings - Fork 833
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: add instrumentation library and update collector exporter #1171
feat: add instrumentation library and update collector exporter #1171
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1171 +/- ##
==========================================
+ Coverage 93.20% 93.24% +0.03%
==========================================
Files 122 122
Lines 3533 3551 +18
Branches 714 714
==========================================
+ Hits 3293 3311 +18
Misses 240 240
|
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.
lgtm, few minor comments
packages/opentelemetry-exporter-collector/test/common/transform.test.ts
Outdated
Show resolved
Hide resolved
@@ -230,6 +230,7 @@ describe('Span', () => { | |||
assert.deepStrictEqual(span.attributes, {}); | |||
assert.deepStrictEqual(span.links, []); | |||
assert.deepStrictEqual(span.events, []); | |||
assert.ok(span.instrumentationLibrary); |
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.
instanceof maybe ?
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.
Since it's a very simple type, there is only an interface. I added more detailed assertions though.
d1bdf89
to
4e65c7a
Compare
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.
Looks good aside from one nit
* feat: add dataloader instrumentation * fix: linter errors and a few mistypings * docs: add component_owners entry for dataloader instrumentation * fix: avoid double shimming of the batch load function * test: cover more lines in tests * refactor: remove prefix from directory name * feat: add configuration option for requiring parent span * chore: drop version number to 0.1.0 * chore: add dataloader instrumentation to release-please-config * feat: add dataloader instrumentation to auto-instrumentations-node * fix: use correct version of Node * fix: add missing export Also make instrumentation export explicit * test: adjust assertion to be correct * chore: change node types to be consistent with other plugins Co-authored-by: Rauno Viskus <[email protected]>
Which problem is this PR solving?
Short description of the changes