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(instr-express): add semconv to readme #1907

Merged

Conversation

JamieDanielson
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • In README, add version of Semantic Conventions used in express instrumentation. See more details re: version in the issue
  • In README, add semantic convention attributes collected

@JamieDanielson
Copy link
Member Author

As mentioned on the issue, I'm hoping to get feedback on this to see if it satisfies the goals of #1778 or if there are pieces to this that are missing. For example, I'm not sure if attributes listed are intended to just be those from semantic conventions, or if all attributes on an instrumented Express span should be listed here.

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Merging #1907 (a7fdfa6) into main (e29b72a) will increase coverage by 0.01%.
The diff coverage is n/a.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1907      +/-   ##
==========================================
+ Coverage   91.02%   91.04%   +0.01%     
==========================================
  Files         146      147       +1     
  Lines        7491     7530      +39     
  Branches     1501     1507       +6     
==========================================
+ Hits         6819     6856      +37     
- Misses        672      674       +2     

see 1 file with indirect coverage changes

@pichlermarc pichlermarc merged commit 44b2df0 into open-telemetry:main Feb 27, 2024
15 checks passed
@pichlermarc
Copy link
Member

As mentioned on the issue, I'm hoping to get feedback on this to see if it satisfies the goals of #1778 or if there are pieces to this that are missing. For example, I'm not sure if attributes listed are intended to just be those from semantic conventions, or if all attributes on an instrumented Express span should be listed here.

I think the goal there is to just to the actual semconv ones, so this PR satisfies the goals of that issue 🙂

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.

None yet

4 participants