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-document-load): use exported strings for semconv #2039

Conversation

JamieDanielson
Copy link
Member

Which problem is this PR solving?

Short description of the changes

In@opentelemetry/instrumentation-document-load package:

  • Update @opentelemetry/semantic-conventions to ^1.22
  • Replace deprecated SemanticAttributes.* with new exported strings SEMATTRS_*
  • Update README with new semantic convention package version and keys

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Merging #2039 (00f43af) into main (dfb2dff) will decrease coverage by 0.31%.
Report is 18 commits behind head on main.
The diff coverage is n/a.

❗ Current head 00f43af differs from pull request most recent head 7e6b606. Consider uploading reports for the commit 7e6b606 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2039      +/-   ##
==========================================
- Coverage   90.97%   90.66%   -0.31%     
==========================================
  Files         146      139       -7     
  Lines        7492     6858     -634     
  Branches     1502     1374     -128     
==========================================
- Hits         6816     6218     -598     
+ Misses        676      640      -36     

see 11 files with indirect coverage changes

@JamieDanielson
Copy link
Member Author

JamieDanielson commented Mar 26, 2024

I got a timeout error suggesting to set/increase browserStartTimeout which defaults to 30000 (30 sec), so I pushed a commit that increases that by 5 seconds... before realizing I probably could've tried to just rerun the failed job (I didn't see the option until the rest of the checks finished). Will see what this does and can revert that change if it's unnecessary - haven't looked yet to see if this is a common "flaky" issue for browser tests or a one-off.

Edit: Removed this in a later commit as it didn't help.

@JamieDanielson
Copy link
Member Author

🤔 Hmm a different error this time:

Failed to launch local browser installed at /usr/bin/google-chrome-stable. This could be because of a mismatch between the version of puppeteer and Chrome or Chromium. Try updating either of them, or adjust the executablePath option to point to another browser installation. Use the --puppeteer flag to run tests with bundled compatible version of Chromium.

It works locally so I'm curious to see what might be causing the error.

➜  opentelemetry-js-contrib git:(jamie.instr-document-load-semconv) npm run test:browser

> opentelemetry-contrib@0.1.0 test:browser
> lerna run test:browser --concurrency 1

lerna notice cli v6.6.2
lerna info versioning independent

    ✔  @opentelemetry/instrumentation-document-load:test:browser (3s)
    ✔  @opentelemetry/instrumentation-user-interaction:test:browser (4s)
    ✔  @opentelemetry/propagator-aws-xray:test:browser (3s)
    ✔  @opentelemetry/id-generator-aws-xray:test:browser (3s)
    ✔  @opentelemetry/instrumentation-long-task:test:browser (3s)
    ✔  @opentelemetry/plugin-react-load:test:browser (4s)
    ✔  @opentelemetry/propagator-instana:test:browser (3s)
    ✔  @opentelemetry/propagator-ot-trace:test:browser (3s)
    ✔  @opentelemetry/auto-instrumentations-web:test:browser (3s)

 ————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Successfully ran target test:browser for 9 projects (29s)

@JamieDanielson
Copy link
Member Author

I added this option in the web-test-runner.config.mjs file to specify a chromeLauncher with --no-sandbox based on the documentation, found from an issue online. Seems to fix the browser test error. If no other ideas I'm inclined to keep it?

browsers: [chromeLauncher({ launchOptions: { args: ['--no-sandbox'] } })],

Looks like this test runner was updated in this package in #1816 so pinging @SimenB in case this seems familiar at all or would cause any known issues.

@SimenB
Copy link
Contributor

SimenB commented Mar 27, 2024

Hey! 👋 I haven't encountered that issue before, but I'm not super familiar with the runner either.

Did you try the puppeteer option like the error suggested?

Use the --puppeteer flag to run tests with bundled compatible version of Chromium.

That seems like what you'd wanna do regardless?

Anyways, if --no-sandbox works, that seems fine as well 🙂

@JamieDanielson
Copy link
Member Author

Did you try the puppeteer option like the error suggested?

Ah, yes I forgot about that. So the only reason I didn't use the --puppeteer flag is because it required another dependency and I wasn't sure if we wanted that. 🤔 I don't have strong feelings there as I'm not super familiar with the test runner, but it seemed good if a flag worked without adding another dependency 😅

Error: You need to add @web/test-runner-puppeteer as a dependency of your project to use the --puppeteer flag.

@pichlermarc
Copy link
Member

Looks good. 👍

Odd that this error surfaced during this PR and not earlier. We ran it quite a few times automatically on main without any hiccups. 🤔

Failed to launch local browser installed at /usr/bin/google-chrome-stable. This could be because of a mismatch between the version of puppeteer and Chrome or Chromium.

I wonder if this second error has something to do with the installed browser version from the image used by the runner. 🤔 I think it's worth to investigate this if we see this happening more frequently.

@pichlermarc pichlermarc merged commit 995b364 into open-telemetry:main Apr 2, 2024
12 checks passed
@JamieDanielson JamieDanielson deleted the jamie.instr-document-load-semconv branch April 2, 2024 16:37
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

5 participants