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

fix: produce screenshots data always #623

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

vigneshshanmugam
Copy link
Member

@vigneshshanmugam vigneshshanmugam commented Oct 6, 2022

  • With the new released patch version of sharp@0.31.1, we started to get errors when extracting data from the main screenshots. Original issue here - Got error "extract_area: bad extract area" when using rotate() when using 0.31.0 lovell/sharp#3352
  • This would be seen in prior releases versions of the synthetics agent if we relied on sharp library with ^0.31.0, as the patch version v0.31.0 would get automatically installed whenever users install or update the synthetics agent. This is the way NPM installation works.
  • Other problem is that we were not seeing it on the local main branch, as we relied on lock files, which was using the sharp@v0.31.0 which did not have the problem.
  • As a result of this issue, all new Heartbeat images which baked in the latest synthetics agent release v0.1.0-beta.36 is unable to produce screenshots as the agent was not writing them correctly.
  • The fix here was to not reuse the old sharp instance, instead create a new sharp instance every-time during extraction which seems to be fix the orientation problem yielding correct results. There is no perf impact due to this change.

Screen Shot 2022-10-06 at 12 38 18 PM

Testing

  1. Create a journey file inline.ts with the below journey
step("Test step", async () => {
  await page.goto("https://www.example.com");
});
  1. Run command cat inline.ts | node dist/cli.js --inline --rich-events | jq .type | uniq to check if you are getting the new screenshots data.
  2. Install this version globally by npm i -g . and check if you can see the data in Kibana by running an inline Heartbeat monitor.
  3. To reproduce the old issue, Undo the code changes in reporters/json.ts and keep the same sharp version v0.31.1, rebuild the agent npm run build.
  4. Run the same command, you would not see the screenshot type in output.

@apmmachine
Copy link
Collaborator

apmmachine commented Oct 6, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-10-06T20:42:46.673+0000

  • Duration: 15 min 3 sec

Test stats 🧪

Test Results
Failed 0
Passed 201
Skipped 2
Total 203

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM if the comment is added, that's a critical comment

src/reporters/json.ts Show resolved Hide resolved
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Not LGTM unfortunately. No screenshots for me:


andrewvc@LAPTOP-BSF625CT ~/projects/synthetics (produce-screeshots)
❯ cat inline.ts | elastic-synthetics --rich-events --inline --screenshots=on  | jq .type | uniq
"synthetics/metadata"
"journey/start"
"step/metrics"
"step/end"
"journey/network_info"
"journey/end"
andrewvc@LAPTOP-BSF625CT ~/projects/synthetics (produce-screeshots)
❯

where the journey is:

  step('Go to https://www.elastic.co/', async () => {
    await page.goto('https://www.elastic.co/');
  });

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM pending that comment

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM. Seeing screenshots.

Screen Shot 2022-10-06 at 4 47 36 PM

cat inline.ts | elastic-synthetics --rich-events --inline --screenshots=on  | jq .type | uniq
"synthetics/metadata"
"journey/start"
"step/metrics"
"step/end"
"screenshot/block"
"step/screenshot_ref"
"journey/network_info"
"journey/end"
dominiqueclarke@DominiquesMBP2 synthetics % cat inline.ts | dist/cli.js --rich-events --inline --screenshots=on  | jq .type | uniq
"synthetics/metadata"
"journey/start"
"step/metrics"
"step/end"
"screenshot/block"
"step/screenshot_ref"
"journey/network_info"
"journey/end"

@vigneshshanmugam vigneshshanmugam merged commit 622a35e into elastic:main Oct 6, 2022
@vigneshshanmugam vigneshshanmugam deleted the produce-screeshots branch October 6, 2022 21:11
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.

None yet

4 participants