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

refactor(instr-restify): use exported strings for attributes #2098

Merged

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Update @opentelemetry/semantic-conventions to ^1.22
  • Replace SemanticAttributes.HTTP_ROUTE with exported string SEMATTRS_HTTP_ROUTE
  • Update README with new semantic convention package version and key

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Merging #2098 (51be0fa) into main (dfb2dff) will decrease coverage by 0.21%.
Report is 55 commits behind head on main.
The diff coverage is 100.00%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2098      +/-   ##
==========================================
- Coverage   90.97%   90.77%   -0.21%     
==========================================
  Files         146      148       +2     
  Lines        7492     7675     +183     
  Branches     1502     1539      +37     
==========================================
+ Hits         6816     6967     +151     
- Misses        676      708      +32     
Files Coverage Δ
...try-instrumentation-restify/src/instrumentation.ts 90.35% <100.00%> (ø)

... and 8 files with indirect coverage changes

@david-luna
Copy link
Contributor Author

david-luna commented Apr 16, 2024

unrelated tests from @opentelemetry/instrumentation-dns failing for node v18 :(

Not happening in other PRs though. Maybe a flaky test?

> @opentelemetry/instrumentation-dns@0.35.0 test
> nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'



  DnsInstrumentation
    unpatch()
      ✓ should not call tracer methods for creating span

  Utility
    satisfiesPattern()
      ✓ string pattern
      ✓ regex pattern
      ✓ should throw if type is unknown
      ✓ function pattern
    isIgnored()
      ✓ should call isSatisfyPattern, n match
      ✓ should call isSatisfyPattern, match for function
      ✓ should call isSatisfyPattern, match for a single matcher
      ✓ should not re-throw when function throws an exception
      ✓ should call onException when function throws an exception
      ✓ should not call isSatisfyPattern
      ✓ should return false on empty list
      ✓ should not throw and return false when list is undefined
    setError()
      ✓ should have error attributes

  dns.lookup()
    with family param
      ✓ should export a valid span with "family" arg to 4
      ✓ should export a valid span with "family" arg to 6
    with no options param
      ✓ should export a valid span
      ✓ should export a valid span with error INVALID_ARGUMENT when "family" param is equal to -1
      ✓ should export a valid span with error INVALID_ARGUMENT when "hostname" param is a number
      extended timeout
        ✓ should export a valid span with error NOT_FOUND
    with options param
      ✓ should export a valid span with "family" to 4
      ✓ should export a valid span when setting "verbatim" property to true and "family" to 4
      ✓ should export a valid span with "family" to 6
      ✓ should export a valid span when setting "verbatim" property to true and "family" to 6
      1) should export a valid span when setting "all" property to true

  dns/promises.lookup()
    with family param
      ✓ should export a valid span with "family" arg to 4
      ✓ should export a valid span with "family" arg to 6
    with no options param
      ✓ should export a valid span
      ✓ should export a valid span with error INVALID_ARGUMENT when "family" param is equal to -1
      ✓ should export a valid span with error INVALID_ARGUMENT when "hostname" param is a number
      extended timeout
        ✓ should export a valid span with error NOT_FOUND
    with options param
      ✓ should export a valid span with "family" to 4
      ✓ should export a valid span when setting "verbatim" property to true and "family" to 4
      ✓ should export a valid span with "family" to 6
      ✓ should export a valid span when setting "verbatim" property to true and "family" to 6
      2) should export a valid span when setting "all" property to true

  dns.promises.lookup()
    with family param
      ✓ should export a valid span with "family" arg to 4 (139ms)
      ✓ should export a valid span with "family" arg to 6
    with no options param
      ✓ should export a valid span
      ✓ should export a valid span with error INVALID_ARGUMENT when "family" param is equal to -1
      ✓ should export a valid span with error INVALID_ARGUMENT when "hostname" param is a number
      extended timeout
        ✓ should export a valid span with error NOT_FOUND
    with options param
      ✓ should export a valid span with "family" to 4
      ✓ should export a valid span when setting "verbatim" property to true and "family" to 4
      ✓ should export a valid span with "family" to 6
      ✓ should export a valid span when setting "verbatim" property to true and "family" to 6
      ✓ should export a valid span when setting "all" property to true


  45 passing (4s)
  2 failing

  1) dns.lookup()
       with options param
         should export a valid span when setting "all" property to true:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-dns/test/integrations/dns-lookup.test.ts)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

  2) dns/promises.lookup()
       with options param
         should export a valid span when setting "all" property to true:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-dns/test/integrations/dns-slash-promises-lookup.test.ts)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)



---------------------|---------|----------|---------|---------|-------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------------|---------|----------|---------|---------|-------------------
All files            |   94.95 |    86.04 |   96.29 |   96.39 |                   
 src                 |   94.82 |    85.36 |   96.15 |   96.29 |                   
  instrumentation.ts |   91.04 |    73.68 |   94.73 |   93.84 | 55,80,119-122     
  utils.ts           |     100 |    95.45 |     100 |     100 | 65                
 src/enums           |     100 |      100 |     100 |     100 |                   
  AttributeNames.ts  |     100 |      100 |     100 |     100 |                   
---------------------|---------|----------|---------|---------|-------------------
npm ERR! Lifecycle script `test` failed with error: 
npm ERR! Error: command failed 
npm ERR!   in workspace: @opentelemetry/instrumentation-dns@0.35.0 
npm ERR!   at location: /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-dns 

@pichlermarc
Copy link
Member

unrelated tests from @opentelemetry/instrumentation-dns failing for node v18 :(

Not happening in other PRs though. Maybe a flaky test?

Yes, this likely a flaky test. I can't recall seeing this specific failure before though.
Let's keep an eye out and open an issue if we see it happening again.

@pichlermarc pichlermarc merged commit 9f5a867 into open-telemetry:main Apr 17, 2024
15 checks passed
@david-luna david-luna deleted the dluna/restify-semconv-strings branch April 17, 2024 15:35
maryliag pushed a commit to maryliag/opentelemetry-js-contrib that referenced this pull request Apr 17, 2024
…lemetry#2098)

* refactor(instr-restify): use exported strings for attributes

* chore(instr-restify): fix lint issues

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
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