Skip to content
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

Adding in support for Hapi JS #323

Merged
merged 5 commits into from
Sep 8, 2020
Merged

Adding in support for Hapi JS #323

merged 5 commits into from
Sep 8, 2020

Conversation

eric-swann-q2
Copy link
Contributor

@eric-swann-q2 eric-swann-q2 commented Jul 29, 2020

Issue #, if available: #94

*Description of changes: Added support for Hapi JS, ported from Hapi-XRay

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bhautikpip bhautikpip requested a review from willarmiros August 4, 2020 00:07
@willarmiros
Copy link
Contributor

Hi @eric-swann-q2,

Thanks for your new contribution. I'll give it a look as soon as I can prioritize it.

@eric-swann-q2
Copy link
Contributor Author

Yo @willarmiros can somebody take a look at this?

@willarmiros willarmiros requested a review from anuraaga August 24, 2020 23:50
@anuraaga
Copy link
Contributor

@eric-swann-q2 So sorry for the silence on this! This is in my to-dos and I'm going to take a look in the next couple of days. Really sorry for the delay.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Just some very minor points related to documentation.

Just wondering, do you happen to have a sample app using this? I'd like to try it out, and we would probably want to incorporate it into documentation at some point. If not, I'll play around with it.

sdk_contrib/hapi/README.md Outdated Show resolved Hide resolved
sdk_contrib/hapi/README.md Show resolved Hide resolved
sdk_contrib/hapi/lib/plugin.js Outdated Show resolved Hide resolved
sdk_contrib/hapi/lib/xray.js Show resolved Hide resolved
sdk_contrib/hapi/package.json Outdated Show resolved Hide resolved
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
@eric-swann-q2
Copy link
Contributor Author

Thanks, this looks great! Just some very minor points related to documentation.

Just wondering, do you happen to have a sample app using this? I'd like to try it out, and we would probably want to incorporate it into documentation at some point. If not, I'll play around with it.

@anuraaga I don't, but I use this in a production app. Could whip something together real quick though...

@eric-swann-q2
Copy link
Contributor Author

OK, @anuraaga, I believe that I've addressed the issues and I also added in a very simple hapi API.

sdk_contrib/hapi/lib/xray.js Outdated Show resolved Hide resolved
sdk_contrib/hapi/test-d/index.test-d.ts.js Outdated Show resolved Hide resolved
sdk_contrib/hapi/package.json Outdated Show resolved Hide resolved
…d plugin state from app to plugins under hapi request
@eric-swann-q2
Copy link
Contributor Author

@cjihrig Thanks for the review, the identified items have been resolved. Please review again.

@ncjones
Copy link

ncjones commented Sep 8, 2020

I integrated a couple of our Hapi apps with XRay last week using the "hapi-xray" package that this is based on. There are three issues I encountered which you may want to consider here:

  1. the plugin is missing a "daemonAddress" option
  2. the plugin is missing an "enabled" option
  3. making a Pino logging adapter is a pain (not specific to this Hapi plugin though)

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

@eric-swann-q2 Thanks for the sample app and cleanups! I could confirm it works nicely.

@ncjones Thanks for the comments. I don't think we need to worry about daemon address in this plugin since it's just instrumentation, not exporting the data itself. The AWS_XRAY_DAEMON_ADDRESS env variable can be used to configure the daemon address

if(!process.env.AWS_XRAY_DAEMON_ADDRESS) {

Is there anything else you need related to the address? We can file an issue for it.

Similar for enabled, since the plugin has to be registered manually I don't think we need a separate enabled for the plugin itself. But we indeed don't have one for the SDK as a whole - I filed #331 for it but let me know if there's anything else you're thinking.

I think we're mostly ready for this PR to be merged will give a bit more time for comments and merge tomorrow :)

@eric-swann-q2
Copy link
Contributor Author

@moonthug Just including you on this as an FYI.

@eric-swann-q2
Copy link
Contributor Author

@willarmiros @anuraaga I see this PR has been approved by not merged. Will this be merged for the next release? Thanks.

@willarmiros willarmiros merged commit 7d6a0ef into aws:master Sep 8, 2020
@willarmiros
Copy link
Contributor

@eric-swann-q2 it is merged and will be included in the next release. Sorry I've been fairly absent from this repo lately as I was involved in another project, but you can expect a release in the near future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants