-
Notifications
You must be signed in to change notification settings - Fork 824
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(plugin-dns): add lookup patches #424
feat(plugin-dns): add lookup patches #424
Conversation
c3a61ad
to
e255799
Compare
Codecov Report
@@ Coverage Diff @@
## master #424 +/- ##
==========================================
- Coverage 95.84% 95.37% -0.48%
==========================================
Files 108 118 +10
Lines 5315 5919 +604
Branches 431 500 +69
==========================================
+ Hits 5094 5645 +551
- Misses 221 274 +53
Continue to review full report at Codecov.
|
4546635
to
43a07ff
Compare
ref: #423 add docs add example include promise patch for lookup Signed-off-by: Olivier Albertini <[email protected]>
43a07ff
to
8c39cde
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.
LGTM ! 👍
) => { | ||
const { code, message, name } = err; | ||
const attributes = { | ||
[AttributeNames.DNS_ERROR_MESSAGE]: message, |
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.
is message a limited set of of possible values or unlimited (e.g. has some unique identifier in it or something)? If it's the latter it should probably go onto the span as an event
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.
I believe we have a limited set. I'm also not sure if people will use this tag for filtering (Pretty sure, they won't for Jaeger since you need to have an exact match). Perhaps we could create an another PR since we could change also http/https plugins as well if we want to create an event instead.
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 💯
…y#424) Without this, spans created in the lambda function are not part of the same trace as the handler span. Co-authored-by: Valentin Marchaud <[email protected]>
Which problem is this PR solving?
Short description of the changes