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

chore: adding instrumentation for connect #588

Merged
merged 17 commits into from Aug 9, 2021
Merged

Conversation

obecny
Copy link
Member

@obecny obecny commented Jul 23, 2021

Which problem is this PR solving?

Short description of the changes

  • Ads auto instrumentation for connect

@obecny obecny added the enhancement New feature or request label Jul 23, 2021
@obecny obecny requested a review from a team as a code owner July 23, 2021 20:30
@obecny obecny self-assigned this Jul 23, 2021
@obecny obecny force-pushed the connect branch 2 times, most recently from e24b3e5 to 07a5341 Compare July 23, 2021 20:36
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #588 (ba64963) into main (e95221b) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head ba64963 differs from pull request most recent head 70c61ce. Consider uploading reports for the commit 70c61ce to get more accurate results

@@            Coverage Diff             @@
##             main     #588      +/-   ##
==========================================
+ Coverage   94.96%   94.98%   +0.01%     
==========================================
  Files         200      199       -1     
  Lines       11787    11849      +62     
  Branches     1122     1108      -14     
==========================================
+ Hits        11194    11255      +61     
- Misses        593      594       +1     
Impacted Files Coverage Δ
...lemetry-instrumentation-document-load/src/utils.ts
...entation-document-load/src/enums/AttributeNames.ts
...metry-instrumentation-document-load/src/version.ts
...strumentation-document-load/src/instrumentation.ts
...trumentation-document-load/src/enums/EventNames.ts
...strumentation-connect/test/instrumentation.test.ts 98.97% <0.00%> (ø)
...entelemetry-instrumentation-connect/src/version.ts 100.00% <0.00%> (ø)
...try-instrumentation-connect/src/instrumentation.ts 98.71% <0.00%> (ø)
...nstrumentation-connect/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

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.

lgtm, just need to fix tests

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Few nits, otherwise beautifully crafted! 👍

examples/connect/server.js Show resolved Hide resolved
examples/connect/server.js Outdated Show resolved Hide resolved
examples/connect/server.js Show resolved Hide resolved
@obecny obecny closed this Aug 2, 2021
@obecny obecny reopened this Aug 2, 2021
@rauno56
Copy link
Member

rauno56 commented Aug 3, 2021

@obecny, see #588 (comment)
There's an example of that in koa tests.

@dyladan
Copy link
Member

dyladan commented Aug 9, 2021

What should we do about the component_owners.yml in the case maintainers add the instrumentation? Is it ok to have an unowned instr or should you own it?

@vmarchaud
Copy link
Member

What should we do about the component_owners.yml in the case maintainers add the instrumentation? Is it ok to have an unowned instr or should you own it?

Well i think we should add them for every instrumentation in case there is a change in maintainer ? Specially since its one of us that have wrote it, it will force us to switch ownership when stepping down.
However it can be done later on in another PR, we need to decide who own each of the instrumentations anyway

@dyladan
Copy link
Member

dyladan commented Aug 9, 2021

What should we do about the component_owners.yml in the case maintainers add the instrumentation? Is it ok to have an unowned instr or should you own it?

Well i think we should add them for every instrumentation in case there is a change in maintainer ? Specially since its one of us that have wrote it, it will force us to switch ownership when stepping down.
However it can be done later on in another PR, we need to decide who own each of the instrumentations anyway

Stepping down as maintainer wouldn't mean stepping down as component owner I think.

@obecny obecny merged commit 3084ff8 into open-telemetry:main Aug 9, 2021
@obecny obecny deleted the connect branch August 9, 2021 21:34
@vmarchaud
Copy link
Member

Stepping down as maintainer wouldn't mean stepping down as component owner I think

Not necessarely i agree, however if a maintainer don't have the time anymore to work on otel i suppose he would not have the time to review PRs about components he owns (in the case of a step down because of lack of time of course).

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

Successfully merging this pull request may close these issues.

New Instrumentation - connect
4 participants