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

feat: Hapi auto-instrumentation #171

Merged
merged 10 commits into from
Aug 18, 2020
Merged

Conversation

carolinee21
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • This instrumentation provides automatic tracing for hapi server routes and (request lifecycle extensions)[https://github.com/hapijs/hapi/blob/master/API.md#request-lifecycle] defined either directly or via a Hapi plugin.

  • Some example traces are shown here: Example Hapi Instrumentation in Zipkin

  • User friendly example code is also added under examples/hapi

  • A visual diagram of the structure of this new instrumentation is provided here:
    Hapi Instrumentation (2)

  • Note: this build is currently affected by the compilation errors which will be fixed via fix: various compilation errors #170

chore: more setup

feat: finished wrapping for server.route

feat: tracing for individual routes

fix: add hapi router tests, add plugin end logic

chore: testing with server

feat: adding instrumentation for Hapi.server, Hapi.Server [sic], and Hapi plugin register function

feat: improve tests + edge case coverage

feat: enforcing that each route/plugin is instrumented at most once

feat: add Hapi example code

chore: update README

Add files via upload

chore: add example images

fix: modifying type definitions and coverage

feat: refactoring to add custom span name and attributes for plugins

chore: add tests for package-based plugins

chore: update example photos

Delete jaeger.jpg

Delete zipkin.jpg

chore: update example photos

chore: update examples

feat: adding instrumentation for server.ext functions

chore: add tests for extensions added within plugin

fix: update example

fix: update example photos to include ext spans

fix: update example code to include request extension instrumentation

chore: update README

docs: add tsdoc comments for Hapi plugin functions

chore: bump version to 0.9.0

chore: fix style

fix: more test updates
@carolinee21 carolinee21 requested a review from a team August 7, 2020 08:48
@carolinee21
Copy link
Contributor Author

@dyladan @obecny If someone could please add the tag "Internship Ending Soon" tag and add a requested review from @open-telemetry/javascript-maintainers, that would be great!

@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #171 into master will increase coverage by 0.62%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   94.61%   95.23%   +0.62%     
==========================================
  Files          84       93       +9     
  Lines        4124     4767     +643     
  Branches      432      493      +61     
==========================================
+ Hits         3902     4540     +638     
- Misses        222      227       +5     
Impacted Files Coverage Δ
...de/opentelemetry-hapi-instrumentation/src/utils.ts 100.00% <0.00%> (ø)
...entelemetry-hapi-instrumentation/test/hapi.test.ts 100.00% <0.00%> (ø)
...de/opentelemetry-hapi-instrumentation/src/types.ts 100.00% <0.00%> (ø)
...de/opentelemetry-hapi-instrumentation/.eslintrc.js 0.00% <0.00%> (ø)
...ode/opentelemetry-hapi-instrumentation/.mocharc.js 60.00% <0.00%> (ø)
...ode/opentelemetry-hapi-instrumentation/src/hapi.ts 98.43% <0.00%> (ø)
.../opentelemetry-hapi-instrumentation/src/version.ts 100.00% <0.00%> (ø)
...etry-hapi-instrumentation/test/hapi-plugin.test.ts 100.00% <0.00%> (ø)
...-hapi-instrumentation/test/hapi-server-ext.test.ts 100.00% <0.00%> (ø)

@carolinee21
Copy link
Contributor Author

Hapi itself is incompatible with Node versions <12.x.x, so the builds using node8 and node10 are expected to fail. Is there a recommended way to handle this and update the CircleCI config so it just checks node12 (and node14 if possible)?

@dyladan
Copy link
Member

dyladan commented Aug 10, 2020

Is there a recommended way to handle this and update the CircleCI config so it just checks node12 (and node14 if possible)?

This would be done directly in the .test.ts files.

For an example of how to skip a test you can look at mysql/pg/mongo which are skipped during normal operation. It goes something like this:

describe('this suite only runs in node 12+', () => {
  if (!semver.satisfies(process.version, '>=12.0.0')) {
    this.skip();
  }

  it('this test is skipped in node 10', () => {});
});

@carolinee21
Copy link
Contributor Author

carolinee21 commented Aug 10, 2020

This would be done directly in the .test.ts files.

Okay, thank you for the response, I'll add that now!

@dyladan
Copy link
Member

dyladan commented Aug 11, 2020

2952602 seems to work, but is there some reason you decided to do it in the mocha config rather than in the tests like I suggested? I'm not saying your approach is worse, just interested in why you decided to do it that way.

@carolinee21
Copy link
Contributor Author

2952602 seems to work, but is there some reason you decided to do it in the mocha config rather than in the tests like I suggested? I'm not saying your approach is worse, just interested in why you decided to do it that way.

@dyladan Yes, there is! The build failure earlier was happening when the Hapi module was imported at the top of each test file, so it was still failing when I tried skipping the individual tests. With the mocha config, it checks the node version before loading any of the test files, so it doesn't run into the same build issues.

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This looks great @carolinee21 thanks for the contribution

@obecny obecny merged commit 58cd219 into open-telemetry:master Aug 18, 2020
@adamegyed adamegyed mentioned this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin for Hapi
6 participants