Skip to content

feat(http,http2): add http context info#1463

Merged
Qard merged 2 commits intoelastic:masterfrom
Qard:expand-http-context
Oct 31, 2019
Merged

feat(http,http2): add http context info#1463
Qard merged 2 commits intoelastic:masterfrom
Qard:expand-http-context

Conversation

@Qard
Copy link
Copy Markdown
Contributor

@Qard Qard commented Oct 23, 2019 •
edited by zube Bot
Loading

Fixes #1462

Checklist

  • Implement code
  • Add tests

@Qard Qard self-assigned this Oct 23, 2019
watson
watson reviewed Oct 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding a drive-by review - I swear I can stop any time if I want 😅

Not sure if my review comment below is valid - could just be me not understanding the http-request-to-url module. Besides that it looks good - just remember to update the TypeScript typings and the docs 👍

Comment thread lib/instrumentation/http-shared.js Outdated
agent.logger.debug('intercepted http.IncomingMessage end event %o', { id: id })
span.end()

httpRequestToUrl(req).then(url => {
Copy link
Copy Markdown
Contributor

@watson watson Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a risk that this promise will never resolve in case req.socket is never populated for some reason?

Copy link
Copy Markdown
Contributor Author

@Qard Qard Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, there was also some test issues due to the timing, so I changed it. The url is fetched out-of-band now, so it'd just be undefined if the socket is never populated.

@Qard Qard force-pushed the expand-http-context branch from 9047e85 to 0fd4548 Compare October 23, 2019 22:03
@Qard
Copy link
Copy Markdown
Contributor Author

Qard commented Oct 23, 2019

I wasn't intending on span.setHttpContext(...) being a public API, so it shouldn't need docs/typings. The existing span.setDbContext(...) is like that already--it's not documented or included in the typings.

@Qard Qard merged commit 555fe48 into elastic:master Oct 31, 2019
@Qard Qard deleted the expand-http-context branch October 31, 2019 00:42
@Qard Qard added the backport:2.x This PR should be backported to the 2.x branch label Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@watson watson watson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Qard Qard

Labels

backport:2.x This PR should be backported to the 2.x branch enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

More info in HTTP spans

2 participants

@Qard @watson
⚡