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

Add HTTP Server and Client duration Metrics in HTTP Node.js Instrumentation #3149

Merged
merged 15 commits into from
Sep 6, 2022

Conversation

hectorhdzg
Copy link
Member

@hectorhdzg hectorhdzg commented Aug 5, 2022

Which problem is this PR solving?

Add HTTP Server and Client duration Metrics in HTTP Node.js Instrumentation.

Fixes #3148

Short description of the changes

Adding support for duration metrics for both incoming and outgoing HTTP request in HTTP instrumentation.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Tested using InMemoryMetricExporter with Node.js Server making external HTTP requests.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@hectorhdzg hectorhdzg requested a review from a team August 5, 2022 22:44
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #3149 (8f622db) into main (a8047ba) will increase coverage by 0.06%.
The diff coverage is 95.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3149      +/-   ##
==========================================
+ Coverage   93.17%   93.24%   +0.06%     
==========================================
  Files         196      196              
  Lines        6494     6541      +47     
  Branches     1371     1373       +2     
==========================================
+ Hits         6051     6099      +48     
+ Misses        443      442       -1     
Impacted Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 94.78% <91.89%> (+0.36%) ⬆️
...es/opentelemetry-instrumentation-http/src/utils.ts 99.17% <100.00%> (+0.10%) ⬆️
...-trace-base/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

};

const startTime = hrTime();
let metricAttributes: MetricAttributes = utils.getIncomingRequestMetricAttributes(spanAttributes, { component: component });
Copy link
Member

Choose a reason for hiding this comment

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

component should already exist on spanAttributes, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

HTTP_SCHEME was not there, so I'm adding as part of this PR as well, it is required according to the spec.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-server-semantic-conventions

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! LGTM

@hectorhdzg
Copy link
Member Author

@dyladan let me know if there is something preventing this one to be merged so I can work on it.

@dyladan
Copy link
Member

dyladan commented Sep 6, 2022

@dyladan let me know if there is something preventing this one to be merged so I can work on it.

Nothing in particular I just haven't had time to review and saw open conversations. I'll take a peek and merge this now.

@dyladan
Copy link
Member

dyladan commented Sep 6, 2022

All conversations appear to have been addressed just weren't marked resolved.

@dyladan dyladan merged commit 597ea98 into open-telemetry:main Sep 6, 2022
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.

Add support for HTTP Server/Client duration Metrics in HTTP instrumentation
5 participants